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

Implement the embedded-hal-async Traits once stabilized #70

Closed
8 of 11 tasks
jessebraham opened this issue Jun 2, 2022 · 24 comments
Closed
8 of 11 tasks

Implement the embedded-hal-async Traits once stabilized #70

jessebraham opened this issue Jun 2, 2022 · 24 comments
Assignees

Comments

@jessebraham
Copy link
Member

jessebraham commented Jun 2, 2022

I believe it's still too early to begin work on this, but once a stable release (or at the very least a more stable alpha release) is available we can revisit this topic.


In [email protected] the traits to implement are:

  • embedded_hal_async::delay::DelayUs
  • embedded_hal_async::digital::Wait
  • embedded_hal_async::i2c::I2c
  • embedded_hal_async::spi::SpiBus
  • embedded_hal_async::spi::SpiBusFlush
  • embedded_hal_async::spi::SpiBusRead
  • embedded_hal_async::spi::SpiBusWrite
  • embedded_hal_async::spi::SpiDevice
  • embedded_hal_async::spi::SpiDeviceRead
  • embedded_hal_async::spi::SpiDeviceWrite
  • embedded_hal_async::serial::Write
@bjoernQ
Copy link
Contributor

bjoernQ commented Jun 2, 2022

I did that for another HAL and I agree it's too early since there are a lot of moving parts, still. But once it's (more) stable I'm happy to see this implemented

Commented on the wrong issue 😄

@bjoernQ
Copy link
Contributor

bjoernQ commented Jun 8, 2022

I think we could and should do this in a separate crate which will use esp-hal under the hood.

That way

  • we don't "destabilize" the HAL in case of (foreseeable) breaking changes
  • we need interrupt handling for the wakers ... I think we would mixup things here if we implement it here
  • we can experiment with it without breaking anything in HAL

For I2C and SPI we will need interrupt support which we don't have yet. But probably we could implement delay and wait for digital inputs any time since we have support for it

@bjoernQ
Copy link
Contributor

bjoernQ commented Jun 14, 2022

I tried something here: https://github.com/bjoernQ/esp32c3-async-rs-experiment
Really just only async gpio input but at least it seems that implementing async HAL in a separate crate is possible and makes sense. The code is terrible but it proves what I wanted to check

@jessebraham
Copy link
Member Author

Cool, thanks for sharing!

Should we just add an esp-hal-async package or something to this repo, or how did you want to go about doing this?

@bjoernQ
Copy link
Contributor

bjoernQ commented Jun 14, 2022

Didn't even think about that - probably a good idea. I'm just not sure if we want one Async-HAL with features for each chip or one Async-HAL for each chip (similar to what we have for esp-hal). The later would be more consistent with esp-hal - really not sure what the best way would be. Also interesting what the progress for #57 and #80 is

@MabezDev
Copy link
Member

MabezDev commented Jan 4, 2023

Working on embedded_hal_async::digital::Wait.

@jessebraham
Copy link
Member Author

digital::Wait implemented in #333.

@jessebraham jessebraham pinned this issue Jan 27, 2023
@MabezDev
Copy link
Member

MabezDev commented Feb 2, 2023

Working on embedded_hal_async::spi::*

@MabezDev
Copy link
Member

MabezDev commented Feb 8, 2023

Async SPI (except spi device) implemented in #385

@JurajSadel
Copy link
Contributor

I'm planning to start working on embedded_hal_async::i2c::I2c soon.

@konkers
Copy link

konkers commented Mar 14, 2023

@JurajSadel Any progress here? I'm looking into working on this and would be happy to pick up any work in progress and/or hear any insight.

@JurajSadel
Copy link
Contributor

Hi, @konkers I haven't spent much time on this yet. Feel free to pick it up.

@JurajSadel
Copy link
Contributor

@konkers Do you have any update regarding async i2c?

@MabezDev MabezDev self-assigned this Mar 31, 2023
@MabezDev
Copy link
Member

Marking DelayUs as complete, it's implemented for embassy-time via the time driver modules.

@konkers
Copy link

konkers commented Mar 31, 2023

@JurajSadel I didn't make too much progress before getting sidetracked. I did some initial digging into the code. The i2c driver doesn't have interrupt support and the current code relies on busy waiting to feed/drain the fifo. The i2c peripheral does not support DMA so the code would need to be refactored into something a bit more state machine like in order to feed/drain the fifo in response the the appropriate fifo interrupts. That's all before adding any of the async support. I'm heading out on vacation next week and may take my esp32 c3 rust board with me to hack on this.

@MabezDev
Copy link
Member

MabezDev commented Apr 5, 2023

Working on SpiDevice with the new eha async release. Added serial::Write to the list too.

@JuliDi
Copy link

JuliDi commented Apr 18, 2023

Marking DelayUs as complete, it's implemented for embassy-time via the time driver modules.

Is there a way to use this with esp-hal-common at the moment? I would like to pass something that implements embedded_hal_async::delay::DelayUs from esp32c3 hal to an async library function.
From what I see, this requires embassy-time version 0.1.1, which requires embedded-hal 1.0.0-alpha.10, which is incompatible with esp-hal-common 0.8.

I presume that using both, alpha.9 and alpha.10 at the same time is not an option. But maybe someone know a workaround?

Details

error: failed to select a version for `embedded-hal`.
    ... required by package `esp-hal-common v0.8.0`
    ... which satisfies dependency `esp-hal-common = "^0.8"` of package `sk6812-esp32c3-example v0.1.0 (/Users/jdickert/Documents/Git/SK6812/examples/esp32c3)`
versions that meet the requirements `=1.0.0-alpha.9` are: 1.0.0-alpha.9

all possible versions conflict with previously selected packages.

  previously selected package `embedded-hal v1.0.0-alpha.10`
    ... which satisfies dependency `embedded-hal-1 = "=1.0.0-alpha.10"` of package `sk6812-esp32c3-example v0.1.0 (/Users/jdickert/Documents/Git/SK6812/examples/esp32c3)`

failed to select a version for `embedded-hal` which could resolve this conflict

@jessebraham
Copy link
Member Author

@MabezDev did you ever make any progress with SpiDevice, or did that work get back-burnered?

@MabezDev
Copy link
Member

MabezDev commented May 2, 2023

@jessebraham back-burnered, I didn't really make a start so feel free to take over :)

@MabezDev
Copy link
Member

MabezDev commented May 4, 2023

I'm working on embedded_hal_async::serial::Write

@jessebraham
Copy link
Member Author

Forgot to comment but I'm working on embedded_hal_async::i2c::I2c

@jessebraham jessebraham self-assigned this May 9, 2023
@elpiel
Copy link
Contributor

elpiel commented Jun 10, 2023

Hello,
I want to raise another issue of the async traits. They've decided not to include an async read crate for UART.
This means that there should be some other impl in the hal crates that supports async reads.

Another point that they've mentioned is using embedded-io Read & Write for UART instead.

See this comment for more details: rust-embedded/embedded-hal#349 (comment)

@jessebraham
Copy link
Member Author

jessebraham commented Jun 10, 2023

Thanks for adding that, we do have a separate tracking issue already for other peripherals: #361

I suppose we should identify which peripherals should/can have async drivers and list them there, though. Haven't updated it in awhile 😁

@jessebraham
Copy link
Member Author

Just as a small update, there is talk of removing the SpiDeviceRead and SpiDeviceWrite traits from embedded-hal-async. For this reason, we're holding off on implementing these final traits until a new release is available.

@jessebraham jessebraham unpinned this issue Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

7 participants