Change semantics of DMA futures

[Dominaezzz]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• My changes were added to the CHANGELOG.md in the proper section.
• My changes are in accordance to the esp-rs API guidelines

Extra:

• I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

This PR is part of #1785, a small part that can be reviewed before the big PR comes in.
I've skipped changelog since this is an internal refactor.

The new semantics in this PR is also a proposal for how most (a handful are a little messy) interrupt based futures should work in the hal, so review this PR with that in mind as well.

The existing semantics of the DMA futures are:

• Set INT_ENA in the constructor.
• Interrupt fires and unsets INT_ENA, clears INT_CLR and wakes waker.
• Poll checks for the unset INT_ENA then returns some result.

The new semantics are:

• Do nothing in constructor. (Rust Futures do nothing unless polled)
• Poll checks for INT_RAW...
• If !=0, clear INT_CLR and returns a result based on what was in INT_RAW.
• Otherwise set INT_ENA and return Poll::Pending.
• Interrupt fires and unsets INT_ENA and wakes waker.
• Poll checks for INT_RAW, clear INT_CLR and returns a result based on what was in INT_RAW.
• Clear INT_ENA in the Drop. (No-op if interrupt fired)

The new semantics are meant to achieve two things.

• Cancel safety
• Eliminate any race conditions

Cancel safety is achieved by the fact that that the interrupt bits are not cleared unless poll returns Poll::Ready(...). So if the future is dropped before (or even after) the interrupt fires, the interrupt bits are still there and can be consumed by some other part of the application if needed. The future can also be re-created if the application wants to try waiting again.

Race conditions are eliminated by not using INT_ENA to check that the interrupt has fired but by instead checking INT_RAW. The existing semantics required you to create the future before starting the DMA, which ran the risk of bugs like #1728 . The new semantics in this PR let you create the future at any point, even after the transfer has started or even after it had finished.

Future enhancements include:

• Marking the futures as !Send. Bad things can happen (before and after this PR) if the interrupt is executed on a different core to where the future is polled.

Testing

I've tested this alongside other changes but not in isolation, I'll need to test this in isolation but it can still be reviewed before that.

[bjoernQ]

I agree, using INT_ENA to signal ready always felt a bit weird. Code looks good so far but I'd like to test things a bit.

No idea what is wrong with CI

[bjoernQ]

This somehow breaks async-I2S-tx in a different way than what was fixed in #1833

[Dominaezzz]

This somehow breaks async-I2S-tx in a different way than what was fixed in #1833

Oh dear. I tested the async SPI and that worked.

What hardware are you testing the I2S example with?

Also I'm guessing it's the circular transfers that are the problem and not the one shot ones? Since those use different futures from the SPI driver.

[bjoernQ]

What hardware are you testing the I2S example with?

You can just run the example without anything attached and see it freezes while it was continuously pushing data before

[bjoernQ]

I2S TX is now working for me, thanks!

I did some more testing and for PARL_IO_RX ( cargo xtask run-example examples esp32c6 embassy_parl_io_rx without anything attached ) it now prints data received in the first iteration but then gets stuck. On main it continuously prints data

[bjoernQ]

LGTM - Thanks

[MabezDev]

Apologies for the delay, this looks like a nice change, thanks!

Could you do a quick rebase on main and then we can merge this :)

[plaes]

Seems like this completely breaks SPI for me on esp32 (revision v3.0).
I'm using this embassy-based project https://github.com/plaes/rust-lilygo-ttgo-lora32

git bisect bad
4ef91c5941883a5dc589209eb91fe25641570686 is the first bad commit
commit 4ef91c5941883a5dc589209eb91fe25641570686 (HEAD)
Author: Dominic Fischer <14130965+Dominaezzz@users.noreply.github.com>
Date:   Wed Jul 24 15:26:39 2024 +0100

    Change semantics of DMA futures (#1835)
...

[bjoernQ]

See #1857