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

DMA-enabled embedded-hal blocking implementations #772

Merged
merged 7 commits into from
Nov 7, 2024

Conversation

jbeaurivage
Copy link
Contributor

@jbeaurivage jbeaurivage commented Oct 23, 2024

Context

SPI timing and performance issues

In #657, @Felix-El has pointed out that the blocking SPI implementations were somewhat fragile and prone to race conditions. @ianrrees also had some similar comments in #751. Essentially, our SPI transfers are done word by word, which might:

  • Introduce buffer overflow errors if the SPI is ran at a too high frequency
  • Break invariants of some timing-dependent protocols build on top of SPI, such as LCD displays and addressable LEDs.

SPI transfers using DMA

Currently, the HAL has minimal support for making SPI transfers using DMA. It exposes the send_with_dma and receive_with_dma methods. However these:

  1. Are non-blocking
  2. Take exclusively 'static buffers in order to satisfy memory safety invariants (namely, due to point 1)
  3. Most importantly, can not be used to build embedded-hal::spi::SpiBus trait implementations.

The same points are also true for our UART and I2C drivers.

Proposal

In this PR, I suggest a new implementation to make DMA-enabled SPI transfers. It provides blocking, safe implementations that don't need to take 'static buffers. The API to use DMA is as simple as configuring 2 DMA channels, then calling Spi::with_dma_channels, and all subsequent transfers will use DMA.

What I propose is that we encourage users who have strict timing or performance requirements, or need very high frequency transfers, to use the DMA implementations. The word-by-word implementation can therefore remain simple, without the need for us to expend lots of energy trying to optimize it for performance. Users with loose timings or lower frequencies can still use it with reduced configuration complexity (which is really not a lot).

The rationale is that we can probably optimize the word-by-word implementation a bit, but we'll soon hit an upper limit, which we can only exceed using DMA anyways. DMA transfers are also impervious to preemption by higher priority interrupts.

I want to add the same mechanisms for I2C and UART, since most of the effort is already done and reusable there.

What's included

  • DMA-enabled blocking SpiBus implementation
  • Mark the various send_with_dma and receive_with_dma methods as deprecated. They should be removed either in this PR or in a subsequent release -- perhaps at the same time as the ehal 0.2 stuff. Keep the UART send_with_dma and receive_with_dma nonblocking methods, as they can be used on a split UART to do some interesting things.
  • Configurable NOP word for SPI transfers Turns out this was already implemented
  • DMA-enabled blocking I2c implementation
  • DMA-enabled blocking Uart implementation

SPI-specific improvements

What's not included

  • embedded-hal-nb DMA implementations. These would either require the user to pass 'static buffers everywhere, or require all DMA-enabled methods to be unsafe. Both options are incompatible with the traits embedded-hal-nb provides. For non-blocking concurrent code, I would strongly suggest users look at async instead (Async API #635).

  • There have also been reports of fragile timing and performance in our blocking embedded-hal v0.2 implementations. I'm not particularly interested in updating those. In fact, I would like to remove embedded-hal v0.2 from atsamd-hal entirely (perhaps in another PR).

Breaking changes

  • Bumps MSRV to Rust 1.77.2

Checklist

  • All new or modified code is well documented, especially public items
  • No new warnings or clippy suggestions have been introduced - CI will deny clippy warnings by default! You may #[allow] certain lints where reasonable, but ideally justify those with a short comment.

Notes for reviewers:

Closes #657.

@jbeaurivage jbeaurivage force-pushed the spi-dma branch 3 times, most recently from 37c34ee to eba50e1 Compare October 24, 2024 02:44
@jbeaurivage jbeaurivage marked this pull request as ready for review October 24, 2024 03:59
@jbeaurivage jbeaurivage force-pushed the spi-dma branch 12 times, most recently from 7b66222 to 3a65edc Compare October 28, 2024 16:46
kyp44 pushed a commit to kyp44/kuboble-rs that referenced this pull request Nov 1, 2024
@jbeaurivage jbeaurivage force-pushed the spi-dma branch 2 times, most recently from 11a130e to 6ada80c Compare November 6, 2024 00:13
Copy link
Member

@sajattack sajattack left a comment

Choose a reason for hiding this comment

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

couple comment typos I trust you'll resolve but LGTM

@jbeaurivage
Copy link
Contributor Author

Thanks for reviewing @sajattack.

There's just one last thing I'm wondering about before I want to close this for good.

In the ehal 0.2 SPI implementation, we (obviously) wait for a DRE every time we want to send a word on the bus, but we never wait for a TXC, even at the end of a transaction.

The new implementation does wait for a TXC after each transaction. I believe that may be unnecessary, according to embedded-hal's SpiBus spec, and potentially even harmful - I suspect it may be a contributor to the timing issues we've been experiencing with SPI. After rereading the spec, turns out flushing should only be necessary when deasserting CS or deinitializing the peripheral - the user should flush manually.

I think I'll remove those bus flushes and test out a bit more before committing to merging this.

@jbeaurivage jbeaurivage force-pushed the spi-dma branch 3 times, most recently from 1d76505 to ebbe7d1 Compare November 7, 2024 17:48
@jbeaurivage jbeaurivage force-pushed the spi-dma branch 3 times, most recently from 768a294 to 9153f65 Compare November 7, 2024 18:10
* Add full support for SPI transfers using DMA and the `embedded_hal::spi::SpiBus` trait
* Deprecate `Spi::send_with_dma` and `Spi::receive_with_dma`
* Add full support for I2C transfers using DMA and the `embedded_hal::i2c::I2c` trait
* Deprecate `I2c::send_with_dma` and `I2c::receive_with_dma`
* Add full support for UART transfers using DMA and the `embedded_io::Write` and `embedded_io::Read` traits
@jbeaurivage
Copy link
Contributor Author

jbeaurivage commented Nov 7, 2024

I've decided to leave the bus flushes alone for now. Seems like removing them introduces some subtle timing bugs, and sometimes even panics. I want to investigate further before touching this, perhaps in a later PR.

@jbeaurivage jbeaurivage merged commit 629caa4 into atsamd-rs:master Nov 7, 2024
108 checks passed
@jbeaurivage jbeaurivage deleted the spi-dma branch November 7, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spi implementation relies on fragile timing
2 participants