esp-hal

esp-hal copied to clipboard

Hello,

this is a WIP to implement the SpiDevice trait from embedded HAL on the esp32. It is currently based on the contents of #101, hence the many duplicate commits.

I'm mainly putting this up here to discuss some strange errors that I get during building my example code. Can someone here please explain to me what these mean? I can't seem to make any sense out of what the compiler tells me...

Here's the error:

error[E0277]: the trait bound `SpinLockMutex<RefCell<esp32_hal::spi::Spi<esp32_hal::esp32::SPI2>>>: Mutex` is not satisfied
   --> examples/spi_eh1_device_loopback.rs:75:40
    |
75  |     let spi_device = SpiBusDevice::new(&mut mutex_spi, cs);
    |                      ----------------- ^^^^^^^^^^^^^^ the trait `Mutex` is not implemented for `SpinLockMutex<RefCell<esp32_hal::spi::Spi<esp32_hal::esp32::SPI2>>>`
    |                      |
    |                      required by a bound introduced by this call
    |
    = help: the following implementations were found:
              <&SpinLockMutex<T> as Mutex>
note: required by a bound in `SpiBusDevice::<'a, B, M, CS>::new`
   --> /var/home/ahartmann/repos/mt/playground/esp-hal/esp-hal-common/src/spi.rs:322:12
    |
322 |         M: Mutex<Data=RefCell<B>>,
    |            ^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `SpiBusDevice::<'a, B, M, CS>::new`

To me this sounds as if SpinLockMutex doesn't implement the xtensa_lx::mutex::Mutex trait. Looking in the docs I see that Mutex is in fact implemented for &SpinLockMutex<T>, a reference to a SpinLockMutex. But I don't get how this works: Why does rust claim that the Mutex here is a value and not a reference? I'm creating it as mut value in the example, but I'm passing it to the code as &mut, which should be a mutable reference, or not?

I currently don't have a solution for your problem but I agree xtensa-lx::Mutex is inconvenient to use In general I guess we should avoid to use xtensa-lx / riscv types in public API since ... we have both, Xtensa and RISCV targets Using those types as an implementation detail is certainly fine

Maybe a solution here might be to encapsulate the usage of the architecture specific synchronization - then the Mutex (btw SpinLockMutex isn't available on ESP32-S2) can be owned by some struct living in the HAL which should make things easier

I agree xtensa-lx::Mutex is inconvenient to use

In fact, it seems to be a re-export of mutex_trait::Mutex as seen here, so it isn't specific to the architecture. I'm still puzzled as to what exactly the error is trying to tell me...

Maybe a solution here might be to encapsulate the usage of the architecture specific synchronization

My idea here was to have the user decide what Mutex they want to have, specific to their usecase. Hence I'm relying only on having the Mutex trait.

What I find really weird is that xtensa-lx implements the Mutex traits only on references of the Mutex types. Why does it do that? How is that supposed to work at all? Because the Mutex is implemented on &T, but the lock() method needs a &mut T.

@bjoernQ I forked xtensa-lx and removed all the &'_ on the Mutex trait implementation. My code compiles now and the example works. It is the weird Mutex type that caused issues. I'll test some of the other ESP32 examples now and if they continue working I'll file a PR against xtensa-lx.

In fact, it seems to be a re-export of mutex_trait::Mutex as seen here, so it isn't specific to the architecture.

Yes, it's that trait ... which is not implemented for many other architectures: https://crates.io/crates/mutex-trait/reverse_dependencies

The way it is implemented makes it inconvenient to use - the trait itself defines just one function

And since we don't have any RISCV implementation of mutex_trait::Mutex we cannot use it for RISCV (and certainly we cannot use anything from xtensa-lx when compiling for RISCV)

Yes, it's that trait ... which is not implemented for many other architectures

Whew. I wasn't aware of that, sorry...

I'll try the struct in the HAL owning the Mutex now. Thanks for the hint!

It works now, I tried to make some more experiments with generic Mutexes, but I'll give up. I haven't yet fenced the code into some conditional compilation gate thingy... so it won't pass CI yet.

Tomorrow I'd like to make some experiments with defining some alias Mutex type so I get away with only fencing that and not all of the other code, too. I'll keep you updated.

Rebased onto main and added conditional compilation fences around the SpiDevice-specific code in the HAL. The fences are necessary because the code takes ownership of the SpiBus instance and wraps it into a Mutex internally. Unfortunately we don't have a shared mutex for all esp variants...

So far it only has example code for the esp32 variant. I can add it to the other variants, too, if you think its helpful.

Keep in mind when reviewing that this is still based on top of the work from #101.

Proudly presenting a finished SpiBusDevice implementation for xtensa32 esp32 variants. Tested with an esp32, don't have access to other esp variants. Due to some shortcomings with the current implementation, the CS line is controlled by software.

I could probably modify the code to have CS hardware-controlled, but that requires quite a bit of hard-coding specific values or adding some intermediate traits. If you have interest I can investigate that, but as you can see from the screenshort it works reasonably well (that's in release mode).

@jessebraham Should work now, still need to copy the example over to all esp variants != esp32. If you're impatient I don't mind you doing that and testing it for me, but I can understand if you have other things to do. I likely won't get a chance to look over it again until monday or so.

I'm 95% confident it should work on all esp variants now. Fingers crossed...

@jessebraham Do you have some recommendations on what GPIOs I can safely use as CS pins on each esp variant except the esp32? I chose 12, 13 and 14 there because they are so conveniently next to each other on my particular breakout board. I don't have any of the others so I don't know which pins are unused there.

I'm not sure it matters too much, everybody is going to have different boards I'd imagine. Generally I use the DevKits from Espressif so that's usually how I determine the pins used. I'll link the pinout diagrams for the boards I have just for reference.

ESP32-C3: https://docs.espressif.com/projects/esp-idf/en/latest/esp32c3/_images/esp32-c3-devkitm-1-v1-pinout.jpg
ESP32-S2: https://docs.espressif.com/projects/esp-idf/en/latest/esp32s2/_images/esp32-s2_saola1-pinout.jpg
ESP32-S3: https://docs.espressif.com/projects/esp-idf/en/latest/esp32s3/_images/ESP32-S3_DevKitC-1_pinlayout_v1.1.jpg

@bjoernQ I don't understand why the CI is failing. Could you please have a look? It's complaining about multiple imports within the same module, but that isn't the case in the file. I can also build the examples locally on my branch and clippy doesn't fail either.

@bjoernQ I don't understand why the CI is failing. Could you please have a look? It's complaining about multiple imports within the same module, but that isn't the case in the file. I can also build the examples locally on my branch and clippy doesn't fail either.

Not sure but I think the CI checks the code as it would look like if it were merged and there were some reformatings in that code - you could try to rebase your branch on main and see if that changes things

Not sure but I think the CI checks the code as it would look like if it were merged

You're right, there were small merge conflicts in the code. Thanks!

@jessebraham There are examples for all esp32 variants now, you should be able to build them via --example spi_eh1_device_loopback --features eh1.

Looking forward to see the results!

This looks awesome. In my quick tests on an esp32-c3 and esp32-s3 everything is working super well

According to CI using critical_section works.

Could you test the implementation on the real silicon, @jessebraham?

It's amazing to see the HAL develop at such an impressive pace. Thanks for your efforts @bjoernQ and @jessebraham !

Whoops, well spotted. Fixed it, so lgtm? :)