SDBlockDevice: revert HW CS support, add async support #181
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JohnK1987
reviewed
Aug 23, 2023
JohnK1987
reviewed
Aug 23, 2023
JohnK1987
previously approved these changes
Aug 23, 2023
multiplemonomials
dismissed
JohnK1987’s stale review
The base branch was changed.
multiplemonomials
force-pushed
the
dev/async-sdblockdevice
branch
from
2d1b27e
to
856e21a
Compare
|
Merging this because it was approved but the approval was removed by a rebase |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
Some time ago, in 0874f74, I updated the SDBlockDevice class to support HW chip selects. My intention was to use this as part of the CI test shield test suite, so that hardware CS functionality could be tested. However, after looking at the SDBlockDevice code and the SPI code more closely, I realized that this cannot work. The reason is, SDBlockDevice relies on keeping CS low across multiple transfers, but that is currently impossible when using hardware CS control with Mbed (even though I bet that most chips support it...).
So, I reverted that commit, expunging the HW CS constructors that were added.
However, for my DMA SPI test suite, I do need to be able to run at least some of the operations in SDBlockDevice with DMA, so that I can prove that DMA works the same as regular SPI. So, this MR converts some of the long-running operations from synchronous SPI calls into transfer_and_wait() calls. I converted every "readily convertable" call, meaning calls where it sends a multibyte buffer over the bus. There are plenty of places where it still uses the single byte API as part of the logic which I don't want to touch as they are part of the logic. However, trying to change that would require a broader refactor of the entire block device which I'm not signing up for (yet!).
The cool thing is, I can do these changes only because !180 adds the ability for asynchronous transfers to respect calls to the
select()anddeselect()functions. Without that feature, async transfers couldn't be used here because they would always bring CS high again after the transfer.At any rate, the goal of this change is to steal back some execution time while it's running long transfers, such as sending/receiving 512 byte data blocks. And, even though it still isn't fully asynchronous, what I have here does that, so I'm good to ship it.
Impact of changes
Migration actions required
Documentation
See class docs
Pull request type
Test results
I ran my new SD block device test suite with this MR and it passed!
Reviewers