qiuchengxuan / async-sdmmc

3 stars 2 forks source link

embedded_hal_async #1

Open elpiel opened 1 year ago

elpiel commented 1 year ago

Thanks for working on this great crate. I was wondering 2 things:

From what I see, this is just raw access crate to SD cards and other crates should build on top of it a filesystem impls (or just raw access to reading/writing with wear leveling and other features)?

PS: Would love to add a README for some info on the project!

qiuchengxuan commented 1 year ago
  • Have you tested the crate on an actual embedded device?

Not yet, just tested on raspberry pi zero with MicroSD connected via SPI

  • It makes more sense to me to now use the embedded_hal_async traits and remove the dependency to alloc.

Since async_trait requires alloc so probably not practical :(

From what I see, this is just raw access crate to SD cards and other crates should build on top of it a filesystem impls (or just raw access to reading/writing with wear leveling and other features)?

Yeah, just raw access for now.

PS: Would love to add a README for some info on the project!

Will upload soon

elpiel commented 1 year ago

Not yet, just tested on raspberry pi zero with MicroSD connected via SPI That's already some testing done, as long as we know reading and writing are working correctly - this is sufficient to my case.

Since async_trait requires alloc so probably not practical :(

Maybe an optional feature that enables an AsyncBus trait in nightly? It's possible to use async in traits with the async_fn_in_trait feature and should remove the need of alloc.

Yeah, just raw access for now.

:heart:

Will upload soon

:heart:

qiuchengxuan commented 1 year ago

Not yet, just tested on raspberry pi zero with MicroSD connected via SPI That's already some testing done, as long as we know reading and writing are working correctly - this is sufficient to my case.

Since async_trait requires alloc so probably not practical :(

Maybe an optional feature that enables an AsyncBus trait in nightly? It's possible to use async in traits with the async_fn_in_trait feature and should remove the need of alloc.

Yeah, just raw access for now.

❤️

Will upload soon

❤️

Oh, really? that's great

elpiel commented 1 year ago

Yes, that it's great indeed. We just have to see how we can integrate async impl into the current impls, still allowing async_trait or maybe we can remove it? But then people must use nightly. And add async bus impls for the SPI

qiuchengxuan commented 1 year ago

Yes, that it's great indeed. We just have to see how we can integrate async impl into the current impls, still allowing async_trait or maybe we can remove it? But then people must use nightly. And add async bus impls for the SPI

Alloc is optional now

elpiel commented 1 year ago

This is awesome, thank you!

have you though about supporting embedded_hal_async traits too?

Right now, for example, bus::Transfer is only implemented for embedded_hal::spi::Transfer (blocking impl):

https://github.com/qiuchengxuan/async-sdmmc/blob/93e58d1b98670398eb2dcaf25deb12a622fa89ac/src/bus/spi/bus.rs#L35-L43

Should be good to impl Transfer for embedded_hal_async traits.

qiuchengxuan commented 1 year ago

This is awesome, thank you!

have you though about supporting embedded_hal_async traits too?

Right now, for example, bus::Transfer is only implemented for embedded_hal::spi::Transfer (blocking impl):

https://github.com/qiuchengxuan/async-sdmmc/blob/93e58d1b98670398eb2dcaf25deb12a622fa89ac/src/bus/spi/bus.rs#L35-L43

Should be good to impl Transfer for embedded_hal_async traits.

ok

elpiel commented 1 year ago

Continuing our conversation - Transfer methods are not async so the library does not compile currently. There's also missed cfg_attr for deasync, as it needs to be added when neither of the async & async-trait features are enabled.

elpiel commented 1 year ago

So, I made this clean up by removing both features and removing the deasync. I'm not even sure why we would want to deasync-ify the crate if the main point is that it provides async support. I've also opted-in for embedded_hal_async trait impls, instead of embedded_hal ones which are not async themselves. This way the crate not only has support for async API but also uses an SPI async impl.

https://github.com/LechevSpace/async-sdmmc/pull/new/move_to_embedded_hal_async

Of course, this is not mandatory for you to merge but I would like us to consider how we can allow async API with the async SPI impl and maybe allow a blocking SPI impl to be used from embedded_hal. Right now, you need nightly but for frameworks like embassy that rely on the same feature (async fn in traits) it seems like a good solution since that's the only framework that support async and it too rely on this feature.

qiuchengxuan commented 1 year ago

So, I made this clean up by removing both features and removing the deasync. I'm not even sure why we would want to deasync-ify the crate if the main point is that it provides async support. I've also opted-in for embedded_hal_async trait impls, instead of embedded_hal ones which are not async themselves. This way the crate not only has support for async API but also uses an SPI async impl.

https://github.com/LechevSpace/async-sdmmc/pull/new/move_to_embedded_hal_async

Of course, this is not mandatory for you to merge but I would like us to consider how we can allow async API with the async SPI impl and maybe allow a blocking SPI impl to be used from embedded_hal. Right now, you need nightly but for frameworks like embassy that rely on the same feature (async fn in traits) it seems like a good solution since that's the only framework that support async and it too rely on this feature.

The name async means by default it's async, not meaning async only, generally I don't think async only lib is a good idea, and for now it seems makes acceptable change to code.

I'll take a look to your PR on this weekend

I'm interested in embassy too, currently I'm using drone-os, while the maintainer are not so cooperative to accept my PR which fixes a critical issue.

elpiel commented 1 year ago

@qiuchengxuan I had a quick email conversation with the maintainer and they said that they are working on a newer (better) version of drone-os, but they are also from Ukraine making things quite less manageable when talking about working on an open source project.

:raised_hands: thanks for taking a look over the weekend! I hope to push this forward and clean-up the code more as I need this for a project I'm working on.

qiuchengxuan commented 1 year ago

https://github.com/qiuchengxuan/async-sdmmc/pull/3

Sorry but I made my own embedded-hal-async support

elpiel commented 1 year ago

3

Sorry but I made my own embedded-hal-async support

Hey, that's great! Let's now try to fix the other outstanding calls with await and build the package with various features to check it out if it works.

Thanks and no need to say sorry about it 😊