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

SPI slave support #469

Open
6 of 7 tasks
LinusU opened this issue Apr 5, 2023 · 12 comments · Fixed by #580 · May be fixed by #2278
Open
6 of 7 tasks

SPI slave support #469

LinusU opened this issue Apr 5, 2023 · 12 comments · Fixed by #580 · May be fixed by #2278
Labels
peripheral:spi SPI peripheral

Comments

@LinusU
Copy link

LinusU commented Apr 5, 2023

(first of all, thanks for all the amazing progress with Rust support for the ESP32, it truly is amazing!)

I'm trying to build an ISP slave device using the ESP32, and of course I want to build it using Rust. However, it seems like there is no support fir this currently?

If I understand correctly, the SPI traits in embedded-hal is only for SPI master, and when slave traits will be added they will be separate and probably named spi_slave. ref: rust-embedded/embedded-hal#396. And as far as I understand, esp-hal is an implementation of embedded-hal?

It seems like the only way right now is to use Rust with the ESP IDF (e.g. using ), and then call the C functions (spi_slave_initialize, spi_slave_transmit, etc.) via the esp_idf_sys crate?

If my understanding is correct, than this is a feature request to add SPI slave support. In either case, any pointers or guidance would be much appreciated!


@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 6, 2023

Yes, we implement SPI traits from embedded-hal which all define only 1 bit full-duplex master mode.
We also have some additional functionality like half-duplex, dual and quad SPI

Technically we can define our own API to add SPI slave support (like we had to do for e.g. I2S before)

TL;DR this is a feature request for SPI slave functionality 😄

@BryanKadzban BryanKadzban mentioned this issue Jun 8, 2023
8 tasks
@jessebraham jessebraham linked a pull request Sep 5, 2023 that will close this issue
8 tasks
@jessebraham jessebraham added the peripheral:spi SPI peripheral label Sep 25, 2023
@jessebraham jessebraham reopened this Oct 10, 2023
@Studiedlist
Copy link
Contributor

As far as I understand from reading #580, slave driver for ESP32 and ESP32-S2 is not implemented due to pdma issues.

I could try to investigate this and add implementation if someone guide me through codebase a little bit so I can understand how to get started

@Studiedlist
Copy link
Contributor

I browsed through available spi options and found some things that could help make it work.
According to esp32::spi0::clock::W documentation and esp32 datasheet, all of its options should be set to 0 in slave mode.
So I added the following to start_read_bytes_dma:

reg_block.clock.modify(|_, w| {
    w.clkdiv_pre().variant(0)
    .clk_equ_sysclk().clear_bit()
    .clkcnt_l().variant(0)
    .clkcnt_h().variant(0)
    .clkcnt_n().variant(0)
});

There are also dma_int_ena::in_done_int_ena and dma_int_ena::in_suc_eof_int_ena bits that enable dma interrupts, but I don't know are they used in current driver.

The next observation is that slave::slv_last_state_bit is working correct, so, I think, dma is handling transaction, but there is something with ch.tx and ch.rx.

Unfortunately, I cannot find actual Rx and Tx implementation since Spi2DmaChannelRxImpl is generated with macro, so I cannot debug rx/tx internals

@Studiedlist
Copy link
Contributor

I have also written sketchy interrupt-based no-cs slave driver:

#[interrupt]
fn GPIO() {
    critical_section::with(|cs| {
        let mut slave = CLK.borrow_ref_mut(cs);
        let slave = slave.as_mut().unwrap();

        let mut timer = TIMER0.borrow_ref_mut(cs);
        let timer = timer.as_mut().unwrap();

        timer.start(100u64.millis());

        let input = slave.mosi.is_high().unwrap();

        slave.end = false;

        slave.buf_n += (input as u8) << (7 - slave.c);
        slave.c += 1;
        if slave.c == 8 {
            slave.c = 0;
            slave.buf.push(slave.buf_n);
            slave.buf_n = 0;
        }
        slave.clk.clear_interrupt();
    });
}

#[interrupt]
fn TG0_T0_LEVEL() {
    critical_section::with(|cs| {
        let mut timer = TIMER0.borrow_ref_mut(cs);
        let timer = timer.as_mut().unwrap();

        let mut slave = CLK.borrow_ref_mut(cs);
        let slave = slave.as_mut().unwrap();

        if slave.end == false {    
            println!("{:?}", slave.buf);
            slave.buf = Vec::with_capacity(110);
            slave.c = 0;
            slave.buf_n = 0;

        }
        slave.end = true;

        if timer.is_interrupt_set() {
            timer.clear_interrupt();
        }
    });
}

It works fine except low speed (about 10kHz).
Could such approach be correct for no-dma implementation?

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 17, 2023

Thanks for working on this!

Regarding the interrupt-based implementation I guess we shouldn't include bit-banging protocol implementations in the HAL. Those could go into a 3rd party crate I guess

@Studiedlist
Copy link
Contributor

Unfortunately, I was unable to figure out what was wrong with ESP32, but I've got ESP32S3, and now test SPI slave with it.
Example is working with Raspberry PI as master, but whenever I change dma_transfer to dma_read, wait call is stuck forever.

I browsed through source code, and spotted strange check at DmaTransfer::is_done for dma_read. I've removed ch.tx.is_done(), and now dma_read is working.
Could it be a mistake, or ch.tx.is_done() is required in case of dma_read?

@Studiedlist
Copy link
Contributor

By the way, current implementation of SPI slave for ESP32S3 is capable of handling insane clock speed of 10mHz.
Thank you all for this feature

@bjoernQ
Copy link
Contributor

bjoernQ commented Dec 11, 2023

Unfortunately, I was unable to figure out what was wrong with ESP32, but I've got ESP32S3, and now test SPI slave with it. Example is working with Raspberry PI as master, but whenever I change dma_transfer to dma_read, wait call is stuck forever.

I browsed through source code, and spotted strange check at DmaTransfer::is_done for dma_read. I've removed ch.tx.is_done(), and now dma_read is working. Could it be a mistake, or ch.tx.is_done() is required in case of dma_read?

I think the intent is to make sure that all bits are clocked out on MOSI - but probably a good hint what to look at. Maybe just checking rx.is_done() should be good enough in reality

@Studiedlist
Copy link
Contributor

Studiedlist commented Dec 11, 2023

Now I see the reason: there is only one implementation of wait for both read and write transfers, so both ch.tx.is_done() and ch.rx.is_done() are required.

According to datasheet, GDMA_OUT_TOTAL_EOF_CH interrupt, which is used to determine if tx channel is done:

triggered when all data corresponding to a linked list (including
multiple descriptors) have been sent via transmit channel n

There is no exact information on whether it is fired when tx list is empty. But, as far as I understand, no transmission takes place when there is nothing to send, so GDMA_OUT_TOTAL_EOF_CH will not be triggered.

I think, the problem can be solved by introducing different SpiDmaTransfer structs for read and write, like SpiDmaTransferRead and SpiDmaTransferWrite.

@Studiedlist
Copy link
Contributor

Please take a look at #1013

@jessebraham jessebraham added the status:needs-attention This should be prioritized label Apr 11, 2024
@jessebraham
Copy link
Member

We're relatively close to the finish line here I think, so let's try to get this issue closed in the next few weeks/months if able :) I will mention this issue during our team meeting on Monday.

@bjoernQ bjoernQ assigned JurajSadel and unassigned bjoernQ May 2, 2024
@MabezDev MabezDev removed the status:needs-attention This should be prioritized label May 2, 2024
@jessebraham jessebraham added the status:in-progress This task is currently being worked on label May 2, 2024
@JurajSadel
Copy link
Contributor

JurajSadel commented May 17, 2024

Update: PR with S2 support #1562. I'm not sure about ESP32, we've spent long time on that but the thing doesn't work.
EDIT: I don't plan to work on ESP32 in a near future.

@JurajSadel JurajSadel removed their assignment May 22, 2024
@jessebraham jessebraham removed the status:in-progress This task is currently being worked on label Jul 8, 2024
@bugadani bugadani linked a pull request Oct 4, 2024 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
peripheral:spi SPI peripheral
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants