Skip to content

Spi/SpiDma: move some fields into global state#5259

Draft
bugadani wants to merge 3 commits intoesp-rs:mainfrom
bugadani:spi-dma
Draft

Spi/SpiDma: move some fields into global state#5259
bugadani wants to merge 3 commits intoesp-rs:mainfrom
bugadani:spi-dma

Conversation

@bugadani
Copy link
Copy Markdown
Contributor

No description provided.

@bugadani
Copy link
Copy Markdown
Contributor Author

/test-size

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 25, 2026

Binary Size Report

esp32

sleep-timer Diff (PR vs. Base)
VM SIZE    
-------------- 
[ = ]       0    TOTAL
Filtering enabled (source_filter); omitted  201Ki of entries

embassy-dhcp Diff (PR vs. Base)
VM SIZE    
-------------- 
[ = ]       0    TOTAL
Filtering enabled (source_filter); omitted  286Ki of entries

@bugadani bugadani force-pushed the spi-dma branch 4 times, most recently from ea449a8 to 9afed61 Compare March 26, 2026 10:28
@github-actions github-actions bot added the merge-conflict Merge conflict detected. Automatically added/removed by CI. label Mar 26, 2026
@github-actions
Copy link
Copy Markdown

New commits in main has made this PR unmergable. Please resolve the conflicts.

@bugadani
Copy link
Copy Markdown
Contributor Author

/test-size loopback_dma_psram

@github-actions
Copy link
Copy Markdown

Triggered binary size analysis for #5259. Results will be posted as a comment when ready.

@github-actions github-actions bot removed the merge-conflict Merge conflict detected. Automatically added/removed by CI. label Mar 26, 2026
@bugadani bugadani marked this pull request as ready for review March 27, 2026 07:02
Copilot AI review requested due to automatic review settings March 27, 2026 07:02
@bugadani bugadani added the skip-changelog No changelog modification needed label Mar 27, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the SPI master (Spi/SpiDma) drivers to move some per-instance fields (pin guards and DMA transfer-in-progress flags) into per-peripheral global state, reducing per-driver storage and centralizing cleanup.

Changes:

  • Introduces SpiWrapper and moves PeripheralGuard ownership into it, with drop-time deinit of shared state.
  • Moves SPI pin guards into the per-peripheral State (via RefCell<SpiPinGuard>), and updates with_* pin assignment APIs to mutate that shared state.
  • Moves DMA “transfer in progress” flags into a per-peripheral global DMA State, and reworks SpiDma to own Spi instead of AnySpi + local flags.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
esp-hal/src/spi/master/mod.rs Moves SPI pin guard tracking into the global per-peripheral State and adds SpiWrapper with drop-time deinit.
esp-hal/src/spi/master/dma.rs Refactors SpiDma to wrap Spi, and moves DMA transfer flags into per-peripheral global DMA state.

Comment on lines 2487 to 2490
pub struct State {
waker: AtomicWaker,
pins: RefCell<SpiPinGuard>,

Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

Storing SpiPinGuard inside a global State behind RefCell means pin connect/disconnect now relies on interior mutability without any explicit critical-section/mutex protection. Given this is global state for a peripheral, it would be safer to use a lock (e.g. NonReentrantMutex<SpiPinGuard>) rather than RefCell, so pin updates are explicitly serialized and don't depend on an unsafe Sync impl.

Copilot uses AI. Check for mistakes.
Comment on lines +1497 to +1504
struct State {
tx_transfer_in_progress: Cell<bool>,
rx_transfer_in_progress: Cell<bool>,
}

// SAFETY: State belongs to the currently constructed driver instance. As such, it'll not be
// accessed concurrently in multiple threads.
unsafe impl Sync for State {}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

unsafe impl Sync for State is not justified here: this DMA State is stored in a static and uses Cell<bool> fields. Cell is not thread-safe and on multi-core chips it can be accessed from multiple contexts if the driver is shared, leading to potential data races/UB. Consider using AtomicBool (or portable_atomic::AtomicBool) for these flags, or wrap the state in an appropriate mutex/critical-section type and avoid the unsafe Sync impl.

Copilot uses AI. Check for mistakes.
Comment on lines +1494 to +1500
struct Info {
dma_peripheral: crate::dma::DmaPeripheral,
}
struct State {
tx_transfer_in_progress: Cell<bool>,
rx_transfer_in_progress: Cell<bool>,
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

The DMA helper types struct Info / struct State introduced here shadow super::Info / super::State imported via use super::*;, which makes the module harder to follow and increases the risk of accidental type confusion later. Consider renaming these to something more specific (e.g. DmaInfo/DmaState) or avoiding the wildcard import.

Copilot uses AI. Check for mistakes.
Comment on lines +2507 to 2508
unsafe impl Sync for State {}

Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

State is made Sync via unsafe impl Sync for State, but it contains RefCell<SpiPinGuard> (and on esp32 also Cell). RefCell/Cell are not thread-safe; on multi-core targets or with interrupt preemption this Sync impl can permit data races/UB if the state is ever accessed from more than one execution context. Prefer a synchronization primitive for pins (e.g. esp_sync::NonReentrantMutex or critical_section::Mutex<RefCell<_>>), or otherwise restructure so State does not need an unsafe Sync impl.

Suggested change
unsafe impl Sync for State {}

Copilot uses AI. Check for mistakes.
@bugadani bugadani marked this pull request as draft March 27, 2026 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog No changelog modification needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants