search

esp-rs / esp-idf-hal

embedded-hal implementation for Rust on ESP32 and ESP-IDF
https://docs.esp-rs.org/esp-idf-hal/
Apache License 2.0
415 stars 170 forks source link

legacy embedded-hal 0.2 trait: Full Duplex SPI Support #3

Open D1plo1d opened 2 years ago

D1plo1d commented 2 years ago

Hey, I'm new to embedded Rust and looking to use https://github.com/smart-leds-rs/ws2812-spi-rs with my esp32 however it looks like the FullDuplex trait that that library needs is commented out in esp-idf-hal - the details of the comment there are unfortunately over my head atm.

Is it possible to open a FullDuplex SPI interface to connect my WS2812 perhaps with some kind of workaround? Is the trait implementation in esp-idf-hal blocked on an issue in embedded-hal?

Relevant Comment: https://github.com/esp-rs/esp-idf-hal/blob/master/src/spi.rs#L469

ivmarkov commented 2 years ago

Sorry for following up so late, I noticed your issue report just now. No it is not possible to uncomment this full-duplex implementation, as it is register-based and is essentially a copy from the esp32-hal crate, which is used for bare-metal (as in when you don't want to use Rust on top of the ESP-IDF framework).

Implementing full-duplex in esp-idf-hal might be an easy job, however I don't own any hardware that can do it, so even if I implement it, I'm not sure (yet) how to really test it. :) If you own such a hardware, I might try to implement something, then you test and give feedback and so on, but that might be a laborious exercise. :)

Let me know if you are still interested in that.

D1plo1d commented 2 years ago

Ah ok, yes that makes sense. I think I've found a work around for my purposes via esp32-hal but I appreciate the detailed response. Thanks @ivmarkov !

MabaKalox commented 1 year ago

Now I am in the same situation. But probably I cant go the same path as @D1plo1d did... I need wifi, and looks like stable working wifi is only available with std... Any plans regarding implementing Full-Duplex spi?

D1plo1d commented 1 year ago

Now I am in the same situation. But probably I cant go the same path as @D1plo1d did... I need wifi, and looks like stable working wifi is only available with std... Any plans regarding implementing Full-Duplex spi?

If you're using an esp32c3 I've managed to work around this by using the new esp-wifi crate (in no_std):

https://github.com/esp-rs/esp-wifi

graphex commented 1 year ago

I'm hitting the same issue with an Adafruit QT Py ESP32-C3 whose purpose is basically to run WS2812 LEDs. I have hardware to help debug, but trying to implement fullDuplex on my own would be a bit too advanced for me. It would be really great to have access to the std lib while also driving smart leds, so if there is anything I could possibly do to help you implement something @ivmarkov please let me know.

MabaKalox commented 1 year ago

Joining @graphex words. @ivmarkov, Implementing by myself would be too advanced for me, but I am ready to help/test. Also I find it not clear, documentation in spi module, states that "Currently only implements full duplex controller mode support."...

graphex commented 1 year ago

As a workaround (completely separating it from this issue—sorry for going off topic) for driving WS2182 LEDs instead of using SPI, the RMT peripheral works well, and there is a smart-led trait implemented for it here. I also tried the patch of the write-only SPI implementation with the ESP32-C3 and it did not work with my hardware.

dberlin commented 1 year ago

The driver itself even defaults to full duplex mode in the config, and the read/write calls are full duplex, just ignoring the other half.

A trivial implementation of the trait seems to just work now?

That is, something that just stores the single word that polling_transmit returns on send and passes it along on read.

At least, i tried it with embedded_sdmmc 3.0 and it worked fine (4.0 no longer requires fullduplex)

It looks like the trait just never got reimplemented.

I'll make a trivial pull request to get this moving

dberlin commented 1 year ago

Actually, this trait totally went away in the latest embedded-hal. It's legacy only.

So rather than submit a PR, i'll attach a patch that will work if for some reason you need spi::FullDuplex

The code otherwise already supports full duplex mode, so this issue should likely be closed. spi.diff.txt

ivmarkov commented 1 year ago

@Vollbrecht Would you review the attached diff w.r.t. whether it is still necessary after your changes?

Vollbrecht commented 1 year ago

@Vollbrecht Would you review the attached diff w.r.t. whether it is still necessary after your changes?

TLDR: With the following quirks in mind i can impl it if someone still needs it.

We currently only impl the blocking/spi traits from e-hal0.2. If someone needs it, its technically still needed. This Full Duplex trait is a bit of a wired one. Question is, if it should be implemented on an BusDriver or an DeviceDriver. It doesn't specify if it has complete ownership of the bus/ allow parallel devices, only that the user has to manage the cs.

Furthermore the user of the trait need's to make sure that he calls read() / write() always one after another but we need to create the memory cell our self for reading and insert it into the write call for it to make an full duplex "transaction" . A bit wired but can be done as shown in the diff, though we would add an internal "buffer" on the BusDriver.

The most problematic i think is performance of the trait in our implementation. Since the read and write calls are only of a single u8 / u16. ( Each new call to polling_transmit has a setup time and if we just send/ read one u8 at a time we will waste a lot of time. With ISR transmit it should be even worse. Though only a problem when people use it as a "fast" bus - Delay between each new read()/write() call would be something in the order of 10us / 100kHz ) If the user is made aware of that i think its fine to implement it.

Vollbrecht commented 1 week ago

We have full support of embedded-hal 1.0 and our Spi interface is a "Full Duplex" one it just doesn't implement the e-hal 0.2 version of this traits. I personally think its not worthwhile to implement because of the above mention limitations.