esp-hal icon indicating copy to clipboard operation
esp-hal copied to clipboard

Implement the `embedded-hal` alpha version Traits

Open jessebraham opened this issue 2 years ago • 6 comments

It should be possible for these to coexist with the existing 0.2.7 implementations. I have already begun work on this.

EDIT: This crate has been broken up into multiple crates, the task list below has been updated to reflect this. Not all new crates are included, they will be added as needed.


  • [email protected]
    • [x] embedded_hal::delay::DelayUs
    • [x] embedded_hal::digital::InputPin
    • [x] embedded_hal::digital::OutputPin
    • [x] embedded_hal::digital::StatefulOutputPin
    • [x] embedded_hal::digital::ToggleableOutputPin
    • [x] embedded_hal::i2c::I2c
    • [ ] embedded_hal::serial::Write
    • [x] embedded_hal::spi::SpiBus
    • [x] embedded_hal::spi::SpiBusFlush
    • [x] embedded_hal::spi::SpiBusRead
    • [x] embedded_hal::spi::SpiBusWrite
    • [x] embedded_hal::spi::SpiDevice
  • [email protected]
    • [x] embedded_hal::serial::Read
    • [x] embedded_hal::serial::Write
    • [x] embedded_hal::spi::FullDuplex
  • [email protected]
    • [ ] embedded_can::blocking::Can
    • [ ] embedded_can::nb::Can

jessebraham avatar Jun 02 '22 17:06 jessebraham

A little over half of these traits have been implemented in #82.

jessebraham avatar Jun 14 '22 15:06 jessebraham

A couple more SPI traits implemented in #95.

jessebraham avatar Jul 04 '22 17:07 jessebraham

Additional SPI traits implemented in #101.

jessebraham avatar Aug 19 '22 18:08 jessebraham

Just a heads up: According to the maintainer of embedded-hal-compat, a new release may be due soon that will add compat for 1.0.0-alpha.8 <-> 0.2.7. Maybe the old traits can be dropped in favor of using the compatibility traits then?

Edit: See https://github.com/ryankurte/embedded-hal-compat/pull/10#issuecomment-1221441361

har7an avatar Aug 22 '22 14:08 har7an

Thanks for the update, I am planning on doing that once we get a stable release of embedded-hal. I believe embedded-hal is still going to have an embedded-hal-nb package split out, so I'd like to just wait until things are stable.

jessebraham avatar Aug 22 '22 14:08 jessebraham

SpiDevice has been implemented in #106

jessebraham avatar Aug 30 '22 13:08 jessebraham

I think SpiDevice holding a critical section throughout a complete transaction is extremely inefficient. If I understand correctly, holding a critical section disables interrupts on the current core (I'm assuming dual core MCUs have independent interrupt masks), and holds a lock across cores. I think this means that using the current implementation a) prevents interrupt handlers running, b) prevents the other core from entering critical sections, and c) makes using multiple SPI peripherals kind of pointless in general, as at most one can be active at any time.

If I'm correct, this is probably less optimal than desired.

bugadani avatar May 03 '23 19:05 bugadani

I think SpiDevice holding a critical section throughout a complete transaction is extremely inefficient.

I agree, this also means we can't currently reuse the device/bus struct's for async versions because it's possible to hold RefCellMut borrows across await points and you'll get a panic if another bus device tries to borrow. We should change this to a proper Mutex to avoid these issues.

MabezDev avatar May 04 '23 10:05 MabezDev

We should change this to a proper Mutex to avoid these issues.

I suspect this will be pretty difficult. I'm not actually sure about details, but if it's possible to use two SpiDevice instances on two RTIC tasks (1 for each), it will be trivially easy to deadlock a firmware - one task waiting for the other to release the Spi bus, without preemption that would make it actually possible. A critical section at least prevents this, though again, I'm not sure if the use case is actually possible or not.

bugadani avatar May 04 '23 10:05 bugadani

A number of traits have been removed, which has been reflected in the original comment. I believe that serial::Write will be going away as well in favour of the embedded-io traits, however I'm not certain on this yet.

jessebraham avatar Aug 14 '23 16:08 jessebraham

The embedded-hal-* ecosystem has moved on to 1.0.0-rc.1, which we already include and implement, so I am going to consider this issue completed and close it.

jessebraham avatar Aug 22 '23 15:08 jessebraham