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

Add spi bus and device options #68

Merged
merged 4 commits into from
May 17, 2022
Merged

Conversation

pyaillet
Copy link
Contributor

@pyaillet pyaillet commented May 8, 2022

Add the possibility to configure:

  • SPI_DEVICE_NO_DUMMY unlocking 80Mhz for write only spi transfers. This alone allowed me to improve a full screen (240*240 pixels) from ~148ms down to ~95ms
  • max_transfer_size and dma_channels which can be used to improve even more the transfer by minimizing the number of transfers and use the DMA capabilities of the idf implementation of spi

Also, the goal is to address #64

src/spi.rs Outdated
@@ -102,6 +102,9 @@ pub mod config {
pub struct Config {
pub baudrate: Hertz,
pub data_mode: embedded_hal::spi::Mode,
pub no_dummy: bool,
pub dma_channel: u32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very unsafe in that the user has to consult the ESP-IDF documentation to figure out what to pass. I would prefer an enum here. Something like:

pub enum DMA {
    Disabled,
    Channel1,
    Channel2,
    Auto,
}

And then From<DMA> implementation for spi_dma_chan_t.

src/spi.rs Outdated
@@ -102,6 +102,9 @@ pub mod config {
pub struct Config {
pub baudrate: Hertz,
pub data_mode: embedded_hal::spi::Mode,
pub no_dummy: bool,
pub dma_channel: u32,
pub max_transfer_size: i32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

i32? I think we should be using usize here.
However the real issue is that the SPI driver code is heavily utilizing this, which means that all transfers - including new shiny DMA - are constrained to multiple transactions of 64 bytes each. What is the point of using DMA in the first place, if we'll use it for small chunks of 64 bytes?

I think the PR must be more comprehensive, in that it should inspect all code-paths, and lift the chunking of 64 bytes restriction where possible. Note that this would not be that easy when your read and write buffers are both non-null and of a different size, but should be still quite possible. And would also allow us to get rid of these on-stack allocated buffers of TRANS_LEN, which seem unnecessary.

@pyaillet
Copy link
Contributor Author

pyaillet commented May 10, 2022

Thank you for your suggestions.
I was totally missing the TRANS_LEN part, but it makes a lot of sense as well as the DMA enum.

Anyway, would it be easier to split this PR ? Addressing only SPI_DEVICE_NO_DUMMY here (unlocking the possibility to use 80Mhz for SPI Bus freq) and taking some more time to handle changes required for proper DMA/Transfer size use ?

@ivmarkov
Copy link
Collaborator

Thank you for your suggestions. I was totally missing the TRANS_LEN part, but it makes a lot of sense as well as the DMA enum.

Anyway, would it be easier to split this PR ? Addressing only SPI_DEVICE_NO_DUMMY here (unlocking the possibility to use 80Mhz for SPI Bus freq) and taking some more time to handle changes required for proper DMA/Transfer size use ?

Absolutely.
Btw, I do realize that the terminology of ESP-IDF itself uses the "dummy" word, but this just looks... dummy. Can we use something more meaningful here in terms of naming. Lower prio though, don't want to nitpick.

@ivmarkov
Copy link
Collaborator

Yeah, I'm struggling with the dummy replacement myself. :( Maybe making it a tad more explicit: no_dummy_bits?

@pyaillet
Copy link
Contributor Author

pyaillet commented May 10, 2022

If we refer to the documentation, maybe something like no_frequency_check or write_only would be better, however it kinds of hide the reality of what's happening under the hood.

@Dominaezzz
Copy link
Contributor

write_only is the way to go IMO. Especially with regards to e-hal 1.0, we can have a specialisation of spi::Master that only implements the write traits. i.e. SpiDevice<Bus = SpiBusWrite>

@Dominaezzz
Copy link
Contributor

I've been sitting on #70 for a while but finally pushed it after seeing this PR, it should help you with the TRANS_LEN bit.
After it lands, it's a one line change in Master::transaction to send bigger transactions for DMA.

@pyaillet
Copy link
Contributor Author

Sorry for the delay on this, I was struggling with odd errors with my test setup :
It's working as expected now.

I kept only the option to control SPI_DEVICE_NO_DUMMY in this PR and renamed it to write_only.
Let me know if I need to fix anything else :)

@ivmarkov ivmarkov merged commit 4be35d7 into esp-rs:master May 17, 2022
@pyaillet pyaillet deleted the spi-options branch May 17, 2022 19:50
maximeborges pushed a commit to yaak-ai/esp-idf-hal that referenced this pull request Jun 7, 2022
…esp-rs#69)

* fixes issue esp-rs#68. reenables compilation of ethernet code

* import some imports into experimental mod
maximeborges pushed a commit to yaak-ai/esp-idf-hal that referenced this pull request Jun 7, 2022
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.

None yet

3 participants