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

Updating SPI CS management #159

Merged
merged 4 commits into from
Nov 3, 2020
Merged

Conversation

ryan-summers
Copy link
Member

This PR fixes #158 by correcting the usage of the SPI management bits.
This PR fixes #71 by working around "endless transaction mode" by allowing more control of the way CS is managed.

This PR also exposes methods to allow SPI to be used in tx-only and rx-only configurations. It also allows for CS to automatically be idled when no data is available in the FIFO for transmission.

@richardeoin
Copy link
Member

Looks good! I tried this out with the spi_dma example on the dma branch.

I somewhat expected that with manage_cs() the !CS pin would go high again after the transaction, but I also need suspend_when_inactive() for that, since the SPI HAL doesn't set CR1.CSUSP or use TSIZE so that SR.EOT is set automatically. It is worth adding a note about this to the manage_cs documentation string? Other than that this PR can be merged.

Potentially this is an area where the SPI HAL could be improved. Adding a suspend() method to Spi<_, Enabled, _> that sets CR1.CSUSP would be a useful companion to manage_cs() when not using the DMA, so that user code can end the transaction manually.

@ryan-summers
Copy link
Member Author

I somewhat expected that with manage_cs() the !CS pin would go high again after the transaction, but I also need suspend_when_inactive() for that, since the SPI HAL doesn't set CR1.CSUSP or use TSIZE so that SR.EOT is set automatically. It is worth adding a note about this to the manage_cs documentation string? Other than that this PR can be merged.

This is an implication of operating SPI in "endless transaction mode" (e.g. TSIZE = 0). The SPI CS pin is held low for the duration of an entire transaction, which means that it is held low indefinitely when in endless transaction mode. I'll add documentation to describe what behavior should be expected.

Note that using TSIZE has some weird implications to the SPI FIFO. Notable, once TSIZE "data" have been written to the SPI FIFO, the TXTF is set, which causes all further TX FIFO writes to be ignored:

Any write to the TxFIFO is ignored when there is no sufficient room to store at least a single data frame (TXP event is
not respected), when TXTF is set or when the SPI is disabled

And the TXTF must be manually cleared by software, so we'd have to continually check + clear the flag in send(), which seems really redundant and not-that-useful.

Potentially this is an area where the SPI HAL could be improved. Adding a suspend() method to Spi<_, Enabled, _> that sets CR1.CSUSP would be a useful companion to manage_cs() when not using the DMA, so that user code can end the transaction manually.

I think it's a good idea as well, although I think it's somewhat unintuitive to "suspend" the transaction - I think a user would just assume that the transaction is over when they are done sending data. "suspend" requires some understanding as to how we are configuring the underlying peripheral. It likely makes more sense to just automatically suspend the transaction after "sending" all of the data, although with how the embedded-hal implements transfer, it's not entirely clear how this would be best accomplished.

@richardeoin
Copy link
Member

Indeed the embedded-hal implementation of Transfer doesn't give us any opportunity to do anything when the transfer completes, which is a pain. So I think it does need to be a separate method.

Anyhow everything in this PR is good, thanks!

bors r+

@bors bors bot merged commit 9f3fe1a into stm32-rs:master Nov 3, 2020
@ryan-summers ryan-summers deleted the rs/issue-158/managed-spi branch November 7, 2020 09:31
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 CS management does not work SPI operates in "endless transaction" mode
2 participants