Skip to content

Conversation

@MabezDev
Copy link
Member

@MabezDev MabezDev commented Jul 22, 2023

An alternative solution to #674, using a wrapper type instead of adding the checks to the current dma operations. This is only implemented for Spi traits at the moment, as that's the only peripheral we have dma support for and e-h traits.

Benefits to this approach:

  • You only pay for it if you need it
  • We can add different e-h impls for different peripherals but reuse this struct
  • The end use can control the scratch buffer size

Negatives to this approach:

  • You have to run into the issue first, and then try to find this wrapper, instead of it being handled seemlessly
  • Instead of more logic in e-h impls, we now have more e-h impls (though most just pass through to the inner impl, except for writes usually)
  • Added type complexity in the end user spi type

Thank you for your contribution.
Please make sure that your submission includes the following:

Must

  • The code compiles without errors or warnings.
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

@bjoernQ
Copy link
Contributor

bjoernQ commented Jul 25, 2023

I like this approach much better than the other PR

@MabezDev
Copy link
Member Author

I like this approach much better than the other PR

Thanks for the feedback! I'll look into fleshing out this approach.

One thing I could use some help with is what to call this thing; I plucked FlashSafeDma out of the air, but its not actually that descriptive. FlashSafeDma only intercepts tx transfers where the source memory is not RAM, so can anyone think of a better name? I suppose we can always embellish the docs to describe its precise functionality.

@MabezDev MabezDev mentioned this pull request Jul 25, 2023
8 tasks
@bugadani
Copy link
Contributor

bugadani commented Jul 25, 2023

FlashSafeDma sounds good enough to me. It should be documented why it's necessary, but otherwise, I find it descriptive enough without being overly verbose.

Edit: maaaaaaybe some users could get confused and think it can also receive into flash (i.e. disambiguating into FlashReadSafeDma may be better, maybe?)?

@MabezDev MabezDev changed the title WIP fix for ROM data transfers using a wrapper type Add FlashSafeDma wrapper for eh traits which ensure correct DMA transfers from flash Jul 26, 2023
@MabezDev MabezDev marked this pull request as ready for review July 26, 2023 10:43
@MabezDev
Copy link
Member Author

FlashSafeDma sounds good enough to me. It should be documented why it's necessary, but otherwise, I find it descriptive enough without being overly verbose.

Edit: maaaaaaybe some users could get confused and think it can also receive into flash (i.e. disambiguating into FlashReadSafeDma may be better, maybe?)?

I've kept it as FlashSafeDma but added some docs to explain what it actually does.

@MabezDev MabezDev requested review from bjoernQ and jessebraham July 26, 2023 10:44
Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing this, the notification must have gotten buried in my inbox because I missed it at some point 😅 I left one small suggestion but overall LGTM, thanks for this!

@jessebraham jessebraham merged commit 47b987f into esp-rs:main Aug 9, 2023
playfulFence pushed a commit to playfulFence/esp-hal that referenced this pull request Sep 26, 2023
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.

4 participants