From 55b9ed1acb243696b049c75493e33d8327baae88 Mon Sep 17 00:00:00 2001 From: Gerzain Mata Date: Sat, 30 May 2026 13:07:11 -0700 Subject: [PATCH 1/3] Make DMA transfer API ownership-based to prevent write-after-submit races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous `start_dma_transfer(&self, framebuffer: &FrameBuf<...>)` released the borrow immediately on return, leaving the caller free to write to the buffer while DMA was still reading it — a data race the compiler could not catch. The new design follows the embedded-hal ecosystem pattern (stm32f4xx-hal, nrf-hal, Embedonomicon): `start_dma_transfer` takes the framebuffer by value and returns a `DmaTransfer` token. The buffer is locked inside the token until `DmaTransfer::wait` returns it, making concurrent access a compile error. Changes: - New `DmaTransfer` trait with `is_done(&self)` and `wait(self) -> Buffer` - New `TransferError` carries the buffer back on failure so it is never lost - `DisplayBackend::start_dma_transfer` now takes `FrameBuf` by value and returns `Result>` - `wait_for_dma`, `is_dma_ready`, `present`, `present_region` removed from the trait — waiting/polling moves to `DmaTransfer`, presenting stays in `SwapChain` - `SimulatorBackend` gains `CompletedTransfer` — an immediately-done token - `SwapChain` restructured with a `FrontState` enum: the front buffer is either `Idle(FrameBuf)` or `InFlight(Transfer)`, preventing any access path to the buffer while DMA is active - `TripleSwapChain` follows the same pattern - All call sites (`dma_rendering_demo.rs`, `present()` etc.) are unchanged --- src/display_backend.rs | 328 +++++++++++++--------- src/swapchain.rs | 619 ++++++++++++++++++++++++----------------- 2 files changed, 567 insertions(+), 380 deletions(-) diff --git a/src/display_backend.rs b/src/display_backend.rs index fc21254..b28e52e 100644 --- a/src/display_backend.rs +++ b/src/display_backend.rs @@ -4,6 +4,16 @@ //! framebuffer transfers using DMA (Direct Memory Access). This enables //! double-buffered rendering where the CPU can render to one buffer while //! the display hardware transfers another buffer. +//! +//! # Safety +//! +//! The key safety property of this API is that `start_dma_transfer` takes +//! ownership of the framebuffer and returns a [`DmaTransfer`] token. The +//! buffer is locked inside the token for the duration of the transfer — +//! the compiler prevents any access to it until [`DmaTransfer::wait`] +//! returns it. This eliminates the data race that arises from the +//! previous borrow-based API, where DMA could be reading from memory that +//! the CPU was free to overwrite. use embedded_graphics_core::pixelcolor::Rgb565; use embedded_graphics_framebuf::{FrameBuf, backends::DMACapableFrameBufferBackend}; @@ -28,105 +38,147 @@ impl DisplayRegion { } } -/// Error types for display backend operations +/// Error types for display backend operations. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum DisplayError { - /// DMA transfer is still in progress + /// DMA transfer is still in progress. Busy, - /// Hardware error during transfer + /// Hardware error during transfer. HardwareError, - /// Invalid buffer configuration + /// Invalid buffer configuration. InvalidBuffer, } -/// Platform-agnostic display backend trait +/// Returned when a DMA transfer fails to start. /// -/// Implementations of this trait handle the hardware-specific details of -/// transferring framebuffer data to the display using DMA. -pub trait DisplayBackend +/// Carries both the error code and the framebuffer back to the caller so +/// the buffer is not lost on failure. +pub struct TransferError where FB: DMACapableFrameBufferBackend, { - /// Start a non-blocking DMA transfer of the framebuffer to the display + /// The framebuffer that could not be transferred. + pub framebuffer: FrameBuf, + /// The reason the transfer failed. + pub error: DisplayError, +} + +/// An in-progress DMA transfer that holds the framebuffer until completion. +/// +/// The buffer is inaccessible while this token is live — the only way to +/// get it back is to call [`wait`](DmaTransfer::wait), which blocks until +/// the hardware has finished reading. +/// +/// Implementors for real hardware should cancel the DMA in their [`Drop`] +/// impl so that dropping a token without waiting is always safe. +pub trait DmaTransfer { + /// The buffer type that is returned when the transfer completes. + type Buffer; + + /// Returns `true` if the DMA hardware has finished the transfer. + fn is_done(&self) -> bool; + + /// Block until the transfer is complete and return the framebuffer. /// - /// # Arguments - /// * `framebuffer` - The framebuffer to transfer + /// Consuming `self` ensures the buffer cannot be accessed while DMA + /// is still reading it. + fn wait(self) -> Self::Buffer; +} + +/// Platform-agnostic display backend trait. +/// +/// Implementations handle the hardware-specific details of transferring a +/// framebuffer to the display. The API is intentionally ownership-based: +/// `start_dma_transfer` takes the framebuffer **by value** and returns a +/// [`DmaTransfer`] token. The buffer is held inside the token and cannot +/// be accessed again until [`DmaTransfer::wait`] returns it. This +/// prevents write-after-submit data races at compile time. +/// +/// # Implementing for real hardware +/// +/// 1. Define a concrete transfer type that holds the buffer and any +/// hardware state needed to poll or cancel the DMA. +/// 2. In `start_dma_transfer`: program the DMA controller, then move the +/// buffer into your transfer type. +/// 3. In `DmaTransfer::wait`: spin or sleep until the done flag is set by +/// the DMA interrupt, then return the buffer. +/// 4. In `DmaTransfer::drop`: if the transfer is still running, cancel +/// it. This ensures that dropping a forgotten token never leaves the +/// DMA controller pointing at freed memory. +pub trait DisplayBackend +where + FB: DMACapableFrameBufferBackend, +{ + /// The transfer token type returned by this backend. + type Transfer: DmaTransfer>; + + /// Start a non-blocking DMA transfer of the full framebuffer. /// - /// # Returns - /// `Ok(())` if transfer started successfully, `Err(DisplayError)` otherwise + /// Takes ownership of the framebuffer. The caller cannot access the + /// buffer again until [`DmaTransfer::wait`] returns it. /// - /// # Note - /// This function should not block. If a transfer is already in progress, - /// it should return `Err(DisplayError::Busy)`. + /// If starting the transfer fails the buffer is returned inside + /// [`TransferError`] so it is not lost. fn start_dma_transfer( &mut self, - framebuffer: &FrameBuf, - ) -> Result<(), DisplayError>; + framebuffer: FrameBuf, + ) -> Result>; - /// Start a non-blocking transfer of a framebuffer sub-region. + /// Start a non-blocking DMA transfer of a framebuffer sub-region. /// - /// Default implementation falls back to full-frame transfer. + /// Backends that do not support partial transfers may ignore `region` + /// and fall back to a full-frame transfer. fn start_dma_transfer_region( &mut self, - framebuffer: &FrameBuf, + framebuffer: FrameBuf, _region: DisplayRegion, - ) -> Result<(), DisplayError> { + ) -> Result> { self.start_dma_transfer(framebuffer) } +} - /// Wait for the current DMA transfer to complete - /// - /// This function blocks until the DMA transfer finishes. - fn wait_for_dma(&mut self); +// ── SimulatorBackend ────────────────────────────────────────────────────────── - /// Check if DMA is ready for a new transfer - /// - /// # Returns - /// `true` if no transfer is in progress, `false` otherwise - fn is_dma_ready(&self) -> bool; +/// A transfer token that is already complete. +/// +/// Used by [`SimulatorBackend`]: since there is no real DMA hardware, the +/// framebuffer is simply held here until `wait` returns it. +pub struct CompletedTransfer +where + FB: DMACapableFrameBufferBackend, +{ + framebuffer: Option>, +} - /// Present a framebuffer to the display (convenience method) - /// - /// This is equivalent to calling `wait_for_dma()` followed by `start_dma_transfer()`. - /// - /// # Arguments - /// * `framebuffer` - The framebuffer to display - /// - /// # Returns - /// `Ok(())` if successful, `Err(DisplayError)` otherwise - fn present(&mut self, framebuffer: &FrameBuf) -> Result<(), DisplayError> { - self.wait_for_dma(); - self.start_dma_transfer(framebuffer) +impl DmaTransfer for CompletedTransfer +where + FB: DMACapableFrameBufferBackend, +{ + type Buffer = FrameBuf; + + fn is_done(&self) -> bool { + true } - /// Present a framebuffer sub-region to the display. - /// - /// Default implementation falls back to full-frame present. - fn present_region( - &mut self, - framebuffer: &FrameBuf, - region: DisplayRegion, - ) -> Result<(), DisplayError> { - self.wait_for_dma(); - self.start_dma_transfer_region(framebuffer, region) + fn wait(mut self) -> FrameBuf { + self.framebuffer + .take() + .expect("CompletedTransfer polled after completion") } } -/// No-op display backend for simulators and testing +/// No-op display backend for simulators and testing. /// -/// This backend immediately "completes" all transfers and is always ready. -/// It's useful for: -/// - Desktop simulators that don't have real DMA hardware +/// All transfers complete immediately — there is no real DMA hardware. +/// Useful for: +/// - Desktop simulators /// - Unit testing swap chain logic /// - Development without target hardware -pub struct SimulatorBackend { - // No state needed for no-op backend -} +pub struct SimulatorBackend; impl SimulatorBackend { - /// Create a new simulator backend pub fn new() -> Self { - Self {} + Self } } @@ -140,24 +192,20 @@ impl DisplayBackend for SimulatorB where FB: DMACapableFrameBufferBackend, { + type Transfer = CompletedTransfer; + fn start_dma_transfer( &mut self, - _framebuffer: &FrameBuf, - ) -> Result<(), DisplayError> { - // No-op: simulator doesn't actually transfer data - Ok(()) - } - - fn wait_for_dma(&mut self) { - // No-op: no real DMA to wait for - } - - fn is_dma_ready(&self) -> bool { - // Always ready since there's no real DMA - true + framebuffer: FrameBuf, + ) -> Result, TransferError> { + Ok(CompletedTransfer { + framebuffer: Some(framebuffer), + }) } } +// ── Tests ───────────────────────────────────────────────────────────────────── + #[cfg(test)] mod tests { extern crate std; @@ -167,9 +215,34 @@ mod tests { use embedded_graphics_framebuf::backends::EndianCorrectedBuffer; use std::vec; - // Type alias for testing type TestBackend = EndianCorrectedBuffer<'static, Rgb565>; + fn make_fb() -> FrameBuf { + use embedded_graphics_framebuf::backends::EndianCorrection; + let data: &'static mut [Rgb565] = vec![Rgb565::BLACK; W * H].leak(); + FrameBuf::new( + EndianCorrectedBuffer::new(data, EndianCorrection::ToLittleEndian), + W, + H, + ) + } + + // ── RegionTrackingBackend ───────────────────────────────────────────────── + + struct RegionTransfer> { + framebuffer: Option>, + } + + impl> DmaTransfer for RegionTransfer { + type Buffer = FrameBuf; + fn is_done(&self) -> bool { + true + } + fn wait(mut self) -> FrameBuf { + self.framebuffer.take().unwrap() + } + } + struct RegionTrackingBackend { region_calls: Cell, } @@ -186,80 +259,85 @@ mod tests { where FB: DMACapableFrameBufferBackend, { + type Transfer = RegionTransfer; + fn start_dma_transfer( &mut self, - _framebuffer: &FrameBuf, - ) -> Result<(), DisplayError> { - Ok(()) + framebuffer: FrameBuf, + ) -> Result, TransferError> { + Ok(RegionTransfer { + framebuffer: Some(framebuffer), + }) } fn start_dma_transfer_region( &mut self, - _framebuffer: &FrameBuf, + framebuffer: FrameBuf, _region: DisplayRegion, - ) -> Result<(), DisplayError> { + ) -> Result, TransferError> { self.region_calls.set(self.region_calls.get() + 1); - Ok(()) - } - - fn wait_for_dma(&mut self) {} - - fn is_dma_ready(&self) -> bool { - true + Ok(RegionTransfer { + framebuffer: Some(framebuffer), + }) } } + // ── Tests ───────────────────────────────────────────────────────────────── + #[test] - fn test_simulator_backend_creation() { - let backend = SimulatorBackend::new(); - // Backend should exist and be ready - // Use explicit trait method call with types - assert!(>::is_dma_ready(&backend)); + fn test_simulator_backend_transfer_completes_immediately() { + let mut backend = SimulatorBackend::new(); + let fb = make_fb::<2, 2>(); + let xfer = >::start_dma_transfer( + &mut backend, + fb, + ) + .unwrap(); + assert!(xfer.is_done()); + let _fb = xfer.wait(); } #[test] - fn test_simulator_backend_always_ready() { + fn test_simulator_backend_returns_buffer_on_wait() { let mut backend = SimulatorBackend::new(); - - // Should always be ready - assert!(>::is_dma_ready(&backend)); - - // Wait should be no-op - >::wait_for_dma(&mut backend); - assert!(>::is_dma_ready(&backend)); + let fb = make_fb::<4, 4>(); + let xfer = >::start_dma_transfer( + &mut backend, + fb, + ) + .unwrap(); + // wait() should return the framebuffer + let fb_back = xfer.wait(); + assert_eq!(fb_back.width, 4); + assert_eq!(fb_back.height, 4); } #[test] - fn test_present_region_calls_region_transfer() { + fn test_region_tracking_backend_counts_region_transfers() { let mut backend = RegionTrackingBackend::new(); - let data = vec![Rgb565::BLACK; 4].leak(); - let fb = FrameBuf::new( - EndianCorrectedBuffer::new( - data, - embedded_graphics_framebuf::backends::EndianCorrection::ToLittleEndian, - ), - 2, - 2, - ); + let fb = make_fb::<2, 2>(); let region = DisplayRegion::new(0, 0, 1, 1); - >::present_region( + let xfer = >::start_dma_transfer_region( &mut backend, - &fb, + fb, region, ) .unwrap(); assert_eq!(backend.region_calls.get(), 1); + let _ = xfer.wait(); + } + + #[test] + fn test_full_transfer_does_not_increment_region_counter() { + let mut backend = RegionTrackingBackend::new(); + let fb = make_fb::<2, 2>(); + let xfer = + >::start_dma_transfer( + &mut backend, + fb, + ) + .unwrap(); + assert_eq!(backend.region_calls.get(), 0); + let _ = xfer.wait(); } } diff --git a/src/swapchain.rs b/src/swapchain.rs index 8dc1152..8bdd21a 100644 --- a/src/swapchain.rs +++ b/src/swapchain.rs @@ -1,81 +1,117 @@ -//! Swap chain implementation for double-buffered rendering +//! Swap chain implementation for double-buffered rendering. //! //! A swap chain manages two framebuffers (front and back) and coordinates //! DMA transfers to eliminate visual tearing and improve performance. //! //! # Architecture -//! - **Back buffer**: CPU renders to this buffer -//! - **Front buffer**: Display hardware reads from this buffer (via DMA) -//! - **Swap**: When rendering completes, buffers are swapped atomically +//! - **Back buffer**: The CPU renders into this buffer at all times. +//! - **Front buffer**: Owned by either an in-flight DMA transfer or the +//! swap chain itself while idle. +//! - **Swap**: When rendering completes, `present()` recovers the front +//! buffer (waiting for any in-flight DMA), swaps it with the back +//! buffer, and hands the new front to the backend to start the next +//! transfer. //! -//! # Benefits -//! - No visual tearing (never display a partially-rendered frame) -//! - Better performance (CPU and display work in parallel) -//! - Predictable frame timing +//! # Safety +//! The front framebuffer is moved into the [`DmaTransfer`] token returned +//! by the backend. The compiler enforces that nobody can write to it until +//! [`DmaTransfer::wait`] returns it, eliminating the data race that a +//! borrow-based API cannot prevent. -use crate::display_backend::{DisplayBackend, DisplayError, DisplayRegion}; +use crate::display_backend::{DisplayBackend, DisplayError, DisplayRegion, DmaTransfer}; use embedded_graphics_core::pixelcolor::Rgb565; use embedded_graphics_framebuf::{ FrameBuf, backends::{DMACapableFrameBufferBackend, EndianCorrectedBuffer, EndianCorrection}, }; -/// Double-buffered swap chain for tear-free rendering +// ── FrontState ──────────────────────────────────────────────────────────────── + +/// Tracks whether the front framebuffer is idle (owned by the swap chain) +/// or in-flight (owned by a DMA transfer token). +enum FrontState { + Idle(FB), + InFlight(Xfer), +} + +impl> FrontState { + /// Recover the framebuffer, blocking if a transfer is still running. + fn recover(self) -> FB { + match self { + FrontState::Idle(fb) => fb, + FrontState::InFlight(xfer) => xfer.wait(), + } + } + + /// Returns `true` if there is no in-flight transfer or it has finished. + fn is_ready(&self) -> bool { + match self { + FrontState::Idle(_) => true, + FrontState::InFlight(xfer) => xfer.is_done(), + } + } +} + +// ── SwapChain ───────────────────────────────────────────────────────────────── + +/// Double-buffered swap chain for tear-free rendering. /// -/// Manages two framebuffers and coordinates swapping between them. -/// Uses a display backend for platform-specific DMA transfers. +/// The back buffer is always available to the CPU via [`get_back_buffer`]. +/// The front buffer moves into the backend's DMA transfer token on each +/// [`present`] call and is recovered (blocking) on the next one. /// /// # Type Parameters -/// - `W`: Framebuffer width in pixels -/// - `H`: Framebuffer height in pixels -/// - `FB`: Framebuffer backend implementing `DMACapableFrameBufferBackend` -/// - `B`: Display backend implementing `DisplayBackend` +/// - `W`, `H`: Framebuffer dimensions in pixels (const generics). +/// - `FB`: Framebuffer backend implementing [`DMACapableFrameBufferBackend`]. +/// - `B`: Display backend implementing [`DisplayBackend`]. +/// +/// [`get_back_buffer`]: SwapChain::get_back_buffer +/// [`present`]: SwapChain::present pub struct SwapChain where FB: DMACapableFrameBufferBackend, B: DisplayBackend, { - /// Front buffer (currently being displayed) - front: FrameBuf, - /// Back buffer (currently being rendered to) + /// Back buffer — always owned by the swap chain. back: FrameBuf, - /// Display backend for DMA transfers + /// Front buffer — either idle here or inside a DMA transfer token. + front: Option, B::Transfer>>, backend: B, - /// Frame counter for statistics frame_count: u64, } -/// Type alias for SwapChain using EndianCorrectedBuffer backend +/// Type alias for `SwapChain` backed by [`EndianCorrectedBuffer`]. /// -/// This is the most common configuration, using statically allocated -/// framebuffer memory with endianness correction. +/// This is the most common configuration for statically allocated +/// framebuffer memory. pub type StandardSwapChain = SwapChain, B>; -/// Constructor for standard swap chain configuration +// ── StandardSwapChain constructor ───────────────────────────────────────────── + impl StandardSwapChain where B: DisplayBackend>, { - /// Create a new swap chain from static slices + /// Create a new swap chain from static slices. /// /// # Arguments - /// * `front_data` - Static mutable slice for front framebuffer - /// * `back_data` - Static mutable slice for back framebuffer - /// * `big_endian` - Whether to use big-endian byte order for colors - /// * `backend` - Display backend for DMA operations + /// * `front_data` — Static mutable slice for the front framebuffer. + /// * `back_data` — Static mutable slice for the back framebuffer. + /// * `big_endian` — Byte order of pixel data sent to the display. + /// * `backend` — Display backend used for DMA operations. /// /// # Example /// ```ignore - /// static mut FB0_DATA: [Rgb565; 800 * 600] = [Rgb565::BLACK; 800 * 600]; - /// static mut FB1_DATA: [Rgb565; 800 * 600] = [Rgb565::BLACK; 800 * 600]; + /// static mut FB0: [Rgb565; 240 * 135] = [Rgb565::BLACK; 240 * 135]; + /// static mut FB1: [Rgb565; 240 * 135] = [Rgb565::BLACK; 240 * 135]; /// /// let swap_chain = unsafe { - /// StandardSwapChain::<800, 600, _>::from_static_slices( - /// &mut FB0_DATA, - /// &mut FB1_DATA, + /// StandardSwapChain::<240, 135, _>::from_static_slices( + /// &mut FB0, + /// &mut FB1, /// false, - /// SimulatorBackend::new(), + /// MyHardwareBackend::new(), /// ) /// }; /// ``` @@ -85,162 +121,186 @@ where big_endian: bool, backend: B, ) -> Self { - let front_backend = if big_endian { - EndianCorrectedBuffer::new(front_data, EndianCorrection::ToBigEndian) - } else { - EndianCorrectedBuffer::new(front_data, EndianCorrection::ToLittleEndian) + let mk_buf = |data: &'static mut [Rgb565]| { + let correction = if big_endian { + EndianCorrection::ToBigEndian + } else { + EndianCorrection::ToLittleEndian + }; + EndianCorrectedBuffer::new(data, correction) }; - let back_backend = if big_endian { - EndianCorrectedBuffer::new(back_data, EndianCorrection::ToBigEndian) - } else { - EndianCorrectedBuffer::new(back_data, EndianCorrection::ToLittleEndian) - }; + let front_fb = FrameBuf::new(mk_buf(front_data), W, H); + let back_fb = FrameBuf::new(mk_buf(back_data), W, H); Self { - front: FrameBuf::new(front_backend, W, H), - back: FrameBuf::new(back_backend, W, H), + back: back_fb, + front: Some(FrontState::Idle(front_fb)), backend, frame_count: 0, } } } +// ── SwapChain methods ───────────────────────────────────────────────────────── + impl SwapChain where FB: DMACapableFrameBufferBackend, B: DisplayBackend, { - /// Get mutable reference to the back buffer for rendering + /// Get a mutable reference to the back buffer for rendering. /// - /// Render all graphics operations to this buffer. When ready to display, - /// call `present()` to swap buffers. + /// The back buffer is always available — DMA only ever touches the + /// front buffer, which is kept separately. pub fn get_back_buffer(&mut self) -> &mut FrameBuf { &mut self.back } - /// Get reference to the front buffer (currently being displayed) + /// Get a reference to the front buffer if it is currently idle. /// - /// This is rarely needed, as you should render to the back buffer. - pub fn get_front_buffer(&self) -> &FrameBuf { - &self.front + /// Returns `None` while a DMA transfer is in progress. + pub fn get_front_buffer(&self) -> Option<&FrameBuf> { + match &self.front { + Some(FrontState::Idle(fb)) => Some(fb), + _ => None, + } } - /// Present the back buffer to the display (blocking) - /// - /// This function: - /// 1. Waits for any in-progress DMA transfer to complete - /// 2. Swaps the front and back buffers - /// 3. Starts a new DMA transfer of the (new) front buffer + /// Present the back buffer (blocking). /// - /// After this call returns, you can immediately start rendering to the - /// (new) back buffer while the display hardware reads the front buffer. + /// 1. Waits for any in-progress DMA transfer to complete. + /// 2. Swaps the front and back buffers. + /// 3. Starts a new DMA transfer of the new front buffer. /// - /// # Returns - /// `Ok(())` if successful, `Err(DisplayError)` if DMA fails + /// After this call returns, the CPU may immediately start rendering to + /// the new back buffer while DMA reads from the new front buffer. pub fn present(&mut self) -> Result<(), DisplayError> { - // Wait for any in-progress DMA to finish - self.backend.wait_for_dma(); - - // Swap front and back buffers - core::mem::swap(&mut self.front, &mut self.back); - - // Start DMA transfer of new front buffer - self.backend.start_dma_transfer(&self.front)?; - - self.frame_count += 1; - Ok(()) + self.present_impl(|backend, fb| backend.start_dma_transfer(fb)) } - /// Try to present the back buffer (non-blocking) + /// Present the back buffer without blocking. /// - /// Like `present()`, but returns immediately with an error if DMA is busy. - /// Useful for variable-rate rendering where you want to skip frames - /// if rendering is too slow. - /// - /// # Returns - /// - `Ok(())` if swap succeeded - /// - `Err(DisplayError::Busy)` if DMA is still transferring - /// - `Err(DisplayError::HardwareError)` on other errors + /// Returns [`DisplayError::Busy`] immediately if a DMA transfer is + /// still in progress, leaving both buffers unchanged. pub fn try_present(&mut self) -> Result<(), DisplayError> { - // Check if DMA is ready - if !self.backend.is_dma_ready() { + if !self.is_ready() { return Err(DisplayError::Busy); } - - // Swap front and back buffers - core::mem::swap(&mut self.front, &mut self.back); - - // Start DMA transfer of new front buffer - self.backend.start_dma_transfer(&self.front)?; - - self.frame_count += 1; - Ok(()) + self.present_impl(|backend, fb| backend.start_dma_transfer(fb)) } - /// Present only a region of the back buffer. + /// Present only a sub-region of the back buffer (blocking). /// - /// Backends that don't support region DMA automatically fall back to full-frame transfer. + /// Backends that do not support partial DMA automatically fall back to + /// a full-frame transfer. pub fn present_region(&mut self, region: DisplayRegion) -> Result<(), DisplayError> { - self.backend.wait_for_dma(); - core::mem::swap(&mut self.front, &mut self.back); - self.backend - .start_dma_transfer_region(&self.front, region)?; - self.frame_count += 1; - Ok(()) + self.present_impl(|backend, fb| backend.start_dma_transfer_region(fb, region)) } /// Non-blocking partial present. + /// + /// Returns [`DisplayError::Busy`] if a transfer is still running. pub fn try_present_region(&mut self, region: DisplayRegion) -> Result<(), DisplayError> { - if !self.backend.is_dma_ready() { + if !self.is_ready() { return Err(DisplayError::Busy); } - core::mem::swap(&mut self.front, &mut self.back); - self.backend - .start_dma_transfer_region(&self.front, region)?; - self.frame_count += 1; - Ok(()) + self.present_impl(|backend, fb| backend.start_dma_transfer_region(fb, region)) } - /// Wait for the current DMA transfer to complete + /// Block until the current DMA transfer completes. /// - /// Useful if you need to ensure a frame has been fully displayed - /// before proceeding (e.g., before taking a screenshot or exiting). + /// After this call the front buffer is in the idle state and the next + /// `present` will not need to wait. pub fn wait_for_vsync(&mut self) { - self.backend.wait_for_dma(); + if let Some(state) = self.front.take() { + let fb = state.recover(); + self.front = Some(FrontState::Idle(fb)); + } } - /// Check if DMA is ready for a new transfer - /// - /// Returns `true` if `try_present()` would succeed, `false` otherwise. + /// Returns `true` if no DMA transfer is running (or the hardware has + /// signalled completion), so `try_present` would succeed. pub fn is_ready(&self) -> bool { - self.backend.is_dma_ready() + self.front.as_ref().map_or(true, |s| s.is_ready()) } - /// Get the number of frames presented + /// Total number of frames presented since construction (or the last + /// [`reset_frame_count`](SwapChain::reset_frame_count) call). pub fn frame_count(&self) -> u64 { self.frame_count } - /// Reset the frame counter + /// Reset the frame counter to zero. pub fn reset_frame_count(&mut self) { self.frame_count = 0; } - /// Get framebuffer dimensions + /// Framebuffer dimensions `(W, H)`. pub fn dimensions(&self) -> (usize, usize) { (W, H) } + + // ── Private helpers ─────────────────────────────────────────────────────── + + /// Shared logic for all present variants. + /// + /// `start_fn` is called with `(&mut backend, front_framebuffer)` and + /// must return a transfer token or a `TransferError`. + fn present_impl(&mut self, start_fn: F) -> Result<(), DisplayError> + where + F: FnOnce( + &mut B, + FrameBuf, + ) -> Result>, + { + // 1. Recover the front framebuffer (block if DMA was running). + let old_front = self + .front + .take() + .expect("SwapChain front buffer missing — double present?") + .recover(); + + // 2. Swap: old_front becomes the new back, current back becomes the + // new front. + let new_front = core::mem::replace(&mut self.back, old_front); + + // 3. Hand the new front to the backend. + match start_fn(&mut self.backend, new_front) { + Ok(transfer) => { + self.front = Some(FrontState::InFlight(transfer)); + self.frame_count += 1; + Ok(()) + } + Err(e) => { + // Transfer failed — put the buffer back so it is not lost. + // self.back currently holds old_front; swap back. + let recovered_front = core::mem::replace(&mut self.back, e.framebuffer); + self.front = Some(FrontState::Idle(recovered_front)); + Err(e.error) + } + } + } } +// ── TripleSwapChain ─────────────────────────────────────────────────────────── + /// Triple-buffered swap chain for smoother pacing under bursty frame times. +/// +/// - `render`: the buffer the CPU is currently writing to. +/// - `ready`: the last fully-rendered buffer waiting to be shown. +/// - `display`: the buffer currently owned by DMA (or idle between frames). +/// +/// On each `present` call the render and display buffers are rotated so +/// the CPU can immediately start the next frame without waiting for the +/// display scan-out to finish. #[cfg(feature = "triple-buffering")] pub struct TripleSwapChain where FB: DMACapableFrameBufferBackend, B: DisplayBackend, { - display: FrameBuf, + display: Option, B::Transfer>>, ready: FrameBuf, render: FrameBuf, backend: B, @@ -264,16 +324,17 @@ where backend: B, ) -> Self { let mk = |data: &'static mut [Rgb565]| { - if big_endian { - EndianCorrectedBuffer::new(data, EndianCorrection::ToBigEndian) + let correction = if big_endian { + EndianCorrection::ToBigEndian } else { - EndianCorrectedBuffer::new(data, EndianCorrection::ToLittleEndian) - } + EndianCorrection::ToLittleEndian + }; + FrameBuf::new(EndianCorrectedBuffer::new(data, correction), W, H) }; Self { - display: FrameBuf::new(mk(display_data), W, H), - ready: FrameBuf::new(mk(ready_data), W, H), - render: FrameBuf::new(mk(render_data), W, H), + display: Some(FrontState::Idle(mk(display_data))), + ready: mk(ready_data), + render: mk(render_data), backend, frame_count: 0, } @@ -286,143 +347,104 @@ where FB: DMACapableFrameBufferBackend, B: DisplayBackend, { + /// Get a mutable reference to the render buffer. pub fn get_render_buffer(&mut self) -> &mut FrameBuf { &mut self.render } + /// Present the render buffer (blocking). + /// + /// Waits for the previous display transfer to complete, then: + /// 1. Rotates `render → display` and starts DMA. + /// 2. Rotates `display (old) → ready` so the CPU has a fresh buffer. pub fn present(&mut self) -> Result<(), DisplayError> { - self.backend.wait_for_dma(); - core::mem::swap(&mut self.display, &mut self.render); - self.backend.start_dma_transfer(&self.display)?; - core::mem::swap(&mut self.ready, &mut self.render); - self.frame_count += 1; - Ok(()) + self.present_impl(|backend, fb| backend.start_dma_transfer(fb)) } + /// Non-blocking triple-buffer present. + /// + /// Returns [`DisplayError::Busy`] if the previous display transfer has + /// not finished yet. pub fn try_present(&mut self) -> Result<(), DisplayError> { - if !self.backend.is_dma_ready() { + let ready = self.display.as_ref().map_or(true, |s| s.is_ready()); + if !ready { return Err(DisplayError::Busy); } - core::mem::swap(&mut self.display, &mut self.render); - self.backend.start_dma_transfer(&self.display)?; - core::mem::swap(&mut self.ready, &mut self.render); - self.frame_count += 1; - Ok(()) + self.present_impl(|backend, fb| backend.start_dma_transfer(fb)) } pub fn frame_count(&self) -> u64 { self.frame_count } + + fn present_impl(&mut self, start_fn: F) -> Result<(), DisplayError> + where + F: FnOnce( + &mut B, + FrameBuf, + ) -> Result>, + { + // 1. Recover the display buffer (block if DMA was running). + let old_display = self + .display + .take() + .expect("TripleSwapChain display buffer missing") + .recover(); + + // 2. render → new display, old_display → render slot temporarily. + let rendered = core::mem::replace(&mut self.render, old_display); + + // 3. Start DMA on the freshly rendered frame. + match start_fn(&mut self.backend, rendered) { + Ok(transfer) => { + self.display = Some(FrontState::InFlight(transfer)); + // 4. ready ↔ render: CPU gets the old ready buffer to render into. + core::mem::swap(&mut self.ready, &mut self.render); + self.frame_count += 1; + Ok(()) + } + Err(e) => { + // Undo: put rendered back into render, restore old_display. + let old_display = core::mem::replace(&mut self.render, e.framebuffer); + self.display = Some(FrontState::Idle(old_display)); + Err(e.error) + } + } + } } +// ── Tests ───────────────────────────────────────────────────────────────────── + #[cfg(test)] mod tests { extern crate std; use super::*; - use crate::display_backend::SimulatorBackend; + use crate::display_backend::{DmaTransfer, SimulatorBackend, TransferError}; use core::cell::Cell; use embedded_graphics_core::pixelcolor::RgbColor; use std::vec; - // Helper to create a leaked slice for testing - fn create_static_buffer() -> &'static mut [Rgb565] { - let vec = vec![Rgb565::BLACK; SIZE]; - vec.leak() - } - - #[test] - fn test_swapchain_creation() { - let fb0 = create_static_buffer::<{ 320 * 240 }>(); - let fb1 = create_static_buffer::<{ 320 * 240 }>(); - - let swap_chain = StandardSwapChain::<320, 240, _>::from_static_slices( - fb0, - fb1, - false, - SimulatorBackend::new(), - ); - - assert_eq!(swap_chain.dimensions(), (320, 240)); - assert_eq!(swap_chain.frame_count(), 0); - assert!(swap_chain.is_ready()); + fn make_static_slice(n: usize) -> &'static mut [Rgb565] { + vec![Rgb565::BLACK; n].leak() } - #[test] - fn test_swapchain_present() { - let fb0 = create_static_buffer::<{ 320 * 240 }>(); - let fb1 = create_static_buffer::<{ 320 * 240 }>(); - - let mut swap_chain = StandardSwapChain::<320, 240, _>::from_static_slices( - fb0, - fb1, - false, - SimulatorBackend::new(), - ); + // ── TrackingBackend ─────────────────────────────────────────────────────── + // + // Counts how many times start_dma_transfer_region was called and + // otherwise behaves like SimulatorBackend. - // Present should succeed - assert!(swap_chain.present().is_ok()); - assert_eq!(swap_chain.frame_count(), 1); + struct TrackingTransfer> { + framebuffer: Option>, } - #[test] - fn test_swapchain_multiple_presents() { - let fb0 = create_static_buffer::<{ 320 * 240 }>(); - let fb1 = create_static_buffer::<{ 320 * 240 }>(); - - let mut swap_chain = StandardSwapChain::<320, 240, _>::from_static_slices( - fb0, - fb1, - false, - SimulatorBackend::new(), - ); - - // Present multiple frames - for _ in 0..5 { - assert!(swap_chain.present().is_ok()); + impl> DmaTransfer for TrackingTransfer { + type Buffer = FrameBuf; + fn is_done(&self) -> bool { + true + } + fn wait(mut self) -> FrameBuf { + self.framebuffer.take().unwrap() } - - assert_eq!(swap_chain.frame_count(), 5); - } - - #[test] - fn test_swapchain_try_present() { - let fb0 = create_static_buffer::<{ 320 * 240 }>(); - let fb1 = create_static_buffer::<{ 320 * 240 }>(); - - let mut swap_chain = StandardSwapChain::<320, 240, _>::from_static_slices( - fb0, - fb1, - false, - SimulatorBackend::new(), - ); - - // try_present should succeed (SimulatorBackend is always ready) - assert!(swap_chain.try_present().is_ok()); - assert_eq!(swap_chain.frame_count(), 1); - } - - #[test] - fn test_swapchain_frame_counter() { - let fb0 = create_static_buffer::<{ 320 * 240 }>(); - let fb1 = create_static_buffer::<{ 320 * 240 }>(); - - let mut swap_chain = StandardSwapChain::<320, 240, _>::from_static_slices( - fb0, - fb1, - false, - SimulatorBackend::new(), - ); - - assert_eq!(swap_chain.frame_count(), 0); - - swap_chain.present().unwrap(); - assert_eq!(swap_chain.frame_count(), 1); - - swap_chain.present().unwrap(); - assert_eq!(swap_chain.frame_count(), 2); - - swap_chain.reset_frame_count(); - assert_eq!(swap_chain.frame_count(), 0); } struct TrackingBackend { @@ -441,49 +463,136 @@ mod tests { where FB: DMACapableFrameBufferBackend, { + type Transfer = TrackingTransfer; + fn start_dma_transfer( &mut self, - _framebuffer: &FrameBuf, - ) -> Result<(), DisplayError> { - Ok(()) + framebuffer: FrameBuf, + ) -> Result, TransferError> { + Ok(TrackingTransfer { + framebuffer: Some(framebuffer), + }) } fn start_dma_transfer_region( &mut self, - _framebuffer: &FrameBuf, + framebuffer: FrameBuf, _region: DisplayRegion, - ) -> Result<(), DisplayError> { + ) -> Result, TransferError> { self.region_present_count .set(self.region_present_count.get() + 1); - Ok(()) + Ok(TrackingTransfer { + framebuffer: Some(framebuffer), + }) } + } - fn wait_for_dma(&mut self) {} + // ── Helper ──────────────────────────────────────────────────────────────── - fn is_dma_ready(&self) -> bool { - true + fn make_swap_chain(backend: B) -> StandardSwapChain<320, 240, B> + where + B: DisplayBackend<320, 240, EndianCorrectedBuffer<'static, Rgb565>>, + { + StandardSwapChain::<320, 240, _>::from_static_slices( + make_static_slice(320 * 240), + make_static_slice(320 * 240), + false, + backend, + ) + } + + // ── SwapChain tests ─────────────────────────────────────────────────────── + + #[test] + fn test_swapchain_creation() { + let sc = make_swap_chain(SimulatorBackend::new()); + assert_eq!(sc.dimensions(), (320, 240)); + assert_eq!(sc.frame_count(), 0); + assert!(sc.is_ready()); + } + + #[test] + fn test_swapchain_present() { + let mut sc = make_swap_chain(SimulatorBackend::new()); + assert!(sc.present().is_ok()); + assert_eq!(sc.frame_count(), 1); + } + + #[test] + fn test_swapchain_multiple_presents() { + let mut sc = make_swap_chain(SimulatorBackend::new()); + for _ in 0..5 { + assert!(sc.present().is_ok()); } + assert_eq!(sc.frame_count(), 5); + } + + #[test] + fn test_swapchain_try_present() { + let mut sc = make_swap_chain(SimulatorBackend::new()); + assert!(sc.try_present().is_ok()); + assert_eq!(sc.frame_count(), 1); + } + + #[test] + fn test_swapchain_frame_counter() { + let mut sc = make_swap_chain(SimulatorBackend::new()); + assert_eq!(sc.frame_count(), 0); + sc.present().unwrap(); + assert_eq!(sc.frame_count(), 1); + sc.present().unwrap(); + assert_eq!(sc.frame_count(), 2); + sc.reset_frame_count(); + assert_eq!(sc.frame_count(), 0); + } + + #[test] + fn test_swapchain_get_back_buffer_always_available() { + let mut sc = make_swap_chain(SimulatorBackend::new()); + sc.present().unwrap(); + // Even after present, back buffer must be accessible for rendering + let _back = sc.get_back_buffer(); + } + + #[test] + fn test_swapchain_wait_for_vsync() { + let mut sc = make_swap_chain(SimulatorBackend::new()); + sc.present().unwrap(); + sc.wait_for_vsync(); + // After vsync, front is idle and is_ready returns true + assert!(sc.is_ready()); } #[test] fn test_swapchain_present_region() { - let fb0 = create_static_buffer::<{ 64 * 64 }>(); - let fb1 = create_static_buffer::<{ 64 * 64 }>(); - let backend = TrackingBackend::new(); - let mut swap_chain = - StandardSwapChain::<64, 64, _>::from_static_slices(fb0, fb1, false, backend); - swap_chain - .present_region(DisplayRegion::new(0, 0, 8, 8)) - .unwrap(); - assert_eq!(swap_chain.backend.region_present_count.get(), 1); + let fb0 = make_static_slice(64 * 64); + let fb1 = make_static_slice(64 * 64); + let mut sc = StandardSwapChain::<64, 64, _>::from_static_slices( + fb0, + fb1, + false, + TrackingBackend::new(), + ); + sc.present_region(DisplayRegion::new(0, 0, 8, 8)).unwrap(); + assert_eq!(sc.backend.region_present_count.get(), 1); } + #[test] + fn test_swapchain_is_ready_after_simulator_present() { + let mut sc = make_swap_chain(SimulatorBackend::new()); + // SimulatorBackend transfer is always done immediately + sc.present().unwrap(); + assert!(sc.is_ready()); + } + + // ── TripleSwapChain tests ───────────────────────────────────────────────── + #[cfg(feature = "triple-buffering")] #[test] fn test_triple_swapchain_present() { - let fb0 = create_static_buffer::<{ 64 * 64 }>(); - let fb1 = create_static_buffer::<{ 64 * 64 }>(); - let fb2 = create_static_buffer::<{ 64 * 64 }>(); + let fb0 = make_static_slice(64 * 64); + let fb1 = make_static_slice(64 * 64); + let fb2 = make_static_slice(64 * 64); let mut sc = StandardTripleSwapChain::<64, 64, _>::from_static_slices( fb0, fb1, From 714d88e8819802496843266484a498f6f136343d Mon Sep 17 00:00:00 2001 From: Gerzain Mata Date: Sat, 30 May 2026 13:10:41 -0700 Subject: [PATCH 2/3] Fix CI: add Debug impl for TransferError, fix height() method call in test --- src/display_backend.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/display_backend.rs b/src/display_backend.rs index b28e52e..86440d1 100644 --- a/src/display_backend.rs +++ b/src/display_backend.rs @@ -63,6 +63,17 @@ where pub error: DisplayError, } +impl core::fmt::Debug for TransferError +where + FB: DMACapableFrameBufferBackend, +{ + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.debug_struct("TransferError") + .field("error", &self.error) + .finish_non_exhaustive() + } +} + /// An in-progress DMA transfer that holds the framebuffer until completion. /// /// The buffer is inaccessible while this token is live — the only way to @@ -308,8 +319,8 @@ mod tests { .unwrap(); // wait() should return the framebuffer let fb_back = xfer.wait(); - assert_eq!(fb_back.width, 4); - assert_eq!(fb_back.height, 4); + assert_eq!(fb_back.width(), 4); + assert_eq!(fb_back.height(), 4); } #[test] From d7d622f7272a971a742610cf6fe344a10d15e2d9 Mon Sep 17 00:00:00 2001 From: Gerzain Mata Date: Sat, 30 May 2026 13:12:35 -0700 Subject: [PATCH 3/3] Add cargo test to pre-commit hook --- .githooks/pre-commit | 47 +++++++++++++------------------------------- 1 file changed, 14 insertions(+), 33 deletions(-) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 0630c87..bdb4b5a 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -1,14 +1,25 @@ #!/usr/bin/env bash +# Pre-commit hook: format staged Rust files and run the test suite. +# +# Activation (one-time per clone): +# git config core.hooksPath .githooks +# +# Behaviour: +# 1. Runs `cargo fmt --all` and re-stages any files that were already staged. +# 2. Runs `cargo test` to catch compile errors and failing tests before the +# commit lands. Only triggered when at least one .rs or Cargo.toml file is +# staged — pure doc/asset commits are unaffected. set -euo pipefail if ! command -v cargo >/dev/null 2>&1; then - echo "pre-commit: cargo not found, skipping rustfmt." >&2 + echo "pre-commit: cargo not found, skipping." >&2 exit 0 fi repo_root="$(git rev-parse --show-toplevel)" cd "$repo_root" +# Collect staged Rust-related files. staged_files="$(git diff --cached --name-only --diff-filter=ACMR)" if [[ -z "$staged_files" ]]; then exit 0 @@ -31,35 +42,5 @@ cargo fmt --all echo "pre-commit: staging rustfmt updates" git add -- "${rust_related[@]}" -#!/bin/sh -# Auto-format staged Rust source with cargo fmt before each commit. -# -# Files already in the index that cargo fmt reformats are re-staged -# automatically so the commit is always formatted without disturbing -# unstaged work in the working tree. -# -# Activation (one-time per clone): -# git config core.hooksPath .githooks - -set -e - -if ! command -v cargo >/dev/null 2>&1; then - echo "pre-commit: cargo not found, skipping fmt" - exit 0 -fi - -# Collect staged .rs paths (added, copied, modified, renamed in the index). -staged_rs=$(git diff --cached --name-only --diff-filter=ACMR | grep '\.rs$' || true) - -if [ -z "$staged_rs" ]; then - exit 0 -fi - -echo "pre-commit: cargo fmt --all" -cargo fmt --all - -# Re-stage only the files that were already staged so that any unstaged edits -# in the working tree are not accidentally included in this commit. -echo "$staged_rs" | while IFS= read -r f; do - [ -f "$f" ] && git add "$f" -done +echo "pre-commit: running cargo test" +cargo test