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

Spi implementation enhancements #751

Open
ianrrees opened this issue Aug 28, 2024 · 1 comment
Open

Spi implementation enhancements #751

ianrrees opened this issue Aug 28, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@ianrrees
Copy link
Contributor

ianrrees commented Aug 28, 2024

Really just notes from a few hours spent with the SPI-driven WS2812 (neopixel) crate.

  1. We currently implement embedded-hal 1.0's SpiBus only for Duplex SPIs, however it could be nice to provide implementations for Tx (and Rx?) SPIs as well, PyGamer provides two examples of peripherals that could use this feature: the WS2812 and the LCD. It's not always possible to provide a non-connected pin to the SPI ctor for an unused Rx or Tx, and a unidirectional implementation could be more efficient than a Duplex one. The impl of methods like SpiBus::read() for Tx SPI should probably panic with an appropriate message.
  2. The implementation we've got now does not take advantage of the pipelines in the SERCOM, so in some cases it stalls between words and is considerably slower than it could be. For example, in the SpiBus::write() impl we have an implementation that doesn't send word N+1 OUT until N has shifted IN:
for word in words {
    let _ = self.transfer_word_in_place(*word)?;
}

but something a little more complicated can have considerably higher throughput, especially when the SPI clock is a higher fraction of the CPU clock (tinkering with a SAMD51, I'm seeing twice as fast in release mode with a 50MHz SPI):

let mut read_count = 0;
for word in words {
    self.block_on_flags(Flags::DRE)?;
    self.config.as_mut().regs.write_data(word.as_());

    if self.read_flags().contains(Flags::RXC) {
        read_count += 1;
        let _ = unsafe { self.read_data().as_() };
    }
}

while read_count < words.len() {
    self.block_on_flags(Flags::RXC)?;
    read_count += 1;
    let _ = unsafe { self.read_data().as_() };
}
  1. For higher-bandwidth transfers, we could think about an implementation that uses DMA. This has an additional benefit that it should precisely control the timing between words, in a way that won't depend on the optimization settings of the compiler. That would help with peripherals like the WS2812, though I'm not sure it would be adequate to fix that particular implementation.
@ianrrees
Copy link
Contributor Author

Thinking more about 1 above; since that behaviour might be a bit unexpected and wouldn't be caught at compile time, it might be better to wrap the unidirectional Spi<_, Tx> in a new PanicOnRead type which implements SpiBus. We could prototype that in a BSP, which might be good enough as it's rather niche.

@ianrrees ianrrees changed the title SpiBus implementation enhancements Spi implementation enhancements Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant