SmartLedsAdapter::new does not have async impl for Rmt::new_async?

[brainstorm]

Mentioned in esp-rs matrix and here too, flagging it here:

error[E0277]: the trait bound `esp_hal::rmt::ChannelCreator<Async, 0>: TxChannelCreator<'_, _, _>` is not satisfied
  --> src/main.rs:27:37
   |
27 |     let led = SmartLedsAdapter::new(rmt.channel0, io.pins.gpio38, rmt_buffer, &clocks);
   |               --------------------- ^^^^^^^^^^^^ the trait `TxChannelCreator<'_, _, _>` is not implemented for `esp_hal::rmt::ChannelCreator<Async, 0>`
   |               |
   |               required by a bound introduced by this call
   |
   = help: the following other types implement trait `TxChannelCreator<'d, T, P>`:
             <esp_hal::rmt::ChannelCreator<Blocking, 0> as TxChannelCreator<'d, esp_hal::rmt::Channel<Blocking, 0>, P>>
             <esp_hal::rmt::ChannelCreator<Blocking, 1> as TxChannelCreator<'d, esp_hal::rmt::Channel<Blocking, 1>, P>>
             <esp_hal::rmt::ChannelCreator<Blocking, 2> as TxChannelCreator<'d, esp_hal::rmt::Channel<Blocking, 2>, P>>
             <esp_hal::rmt::ChannelCreator<Blocking, 3> as TxChannelCreator<'d, esp_hal::rmt::Channel<Blocking, 3>, P>>

[bjoernQ]

I somehow missed that on Matrix.

There is no async-version of smart_leds_trait AFAIK but probably we can just implement an async-write on SmartLedsAdapter

[brainstorm]

Thanks! Perhaps worth opening another issue/discussion although applies to this particular case too:

What do you think about the use of ::new() builder for both blocking and async? As mentioned over matrix:

Regarding blocking vs async ::new() methods in esp_hal, I've observed that there's I2s::new() which is is async (when feature-gated w/ async), but then there's Uart::new_async_with_config and Rmt::new_async() (instead of just using ::new() for both modes like in the I2s case).

I personally like ::new() for both blocking and async if possible, so if I have to implement SmartLedsAdapter's new async builder method which variant would be "preferred"?:

SmartLedsAdapterAsync::new()
SmartLedsAdapter::new()            # <-- with features = ["async"], <3
SmartLedsAdapter::new_async()

Is it worth making this uniform across peripherals in esp-hal or is it too painful of a breaking change at this point?

/cc @MabezDev

[Dominaezzz]

Seeing as the SmartLedsAdapter struct itself doesn't provide the async-ness, but the rmt channel does. It makes sense to just have a single new() function, then the write method can be conditionally async or blocking depending on what the rmt channel's config is.

(Though one could argue that since this struct's sole purpose was to provide an adapter for smart_leds, maybe it shouldn't have an async variant without an upstream trait, but no reason to be strict I guess)

Is it worth making this uniform across peripherals in esp-hal or is it too painful of a breaking change at this point?

This could probably be done for the drivers that mandate DMA, as the async mode could be inferred from the dma channel as well. (Personally I'd say it's too soon as the DMA based apis aren't mature enough)

[brainstorm]

Thanks for the comments and ideas!

Seeing as the SmartLedsAdapter struct itself doesn't provide the async-ness

Indeed, that Rmt struct member, Option<TX>, where TX is pointing to pub trait TxChannel: private::TxChannelInternal<crate::Blocking> does seem to seal the deal on blocking, unfortunately :_/

(...) then the write method can be conditionally async or blocking

I'm not sure if limiting the async-ness to just the write method as @Dominaezzz suggests would help for too long since the RX side would eventually need async as well (for other applications or even for, say, RMT async-driven RX code for photosensors (or LEDs being used as photosensors :P)?

For now I'm being cheeky, breaking stuff locally, patching and defaulting to async for write (don't know if the #cfg's/conditionals/gating needed here would be accepted as "clean" or good?):

on smartleds-esp-hal:

diff --git a/esp-hal-smartled/src/lib.rs b/esp-hal-smartled/src/lib.rs
index 98164779..86a91249 100644
--- a/esp-hal-smartled/src/lib.rs
+++ b/esp-hal-smartled/src/lib.rs
@@ -30,7 +30,7 @@ use esp_hal::{
     clock::Clocks,
     gpio::OutputPin,
     peripheral::Peripheral,
-    rmt::{Error as RmtError, PulseCode, TxChannel, TxChannelConfig, TxChannelCreator},
+    rmt::{asynch::TxChannelAsync, Error as RmtError, PulseCode, TxChannelConfig, TxChannelCreatorAsync},
 };
 use smart_leds_trait::{SmartLedsWrite, RGB8};

@@ -73,7 +73,7 @@ macro_rules! smartLedBuffer {
 /// interaction functionality using the `smart-leds` crate
 pub struct SmartLedsAdapter<TX, const BUFFER_SIZE: usize>
 where
-    TX: TxChannel,
+    TX: TxChannelAsync,
 {
     channel: Option<TX>,
     rmt_buffer: [u32; BUFFER_SIZE],
@@ -82,7 +82,7 @@ where

 impl<'d, TX, const BUFFER_SIZE: usize> SmartLedsAdapter<TX, BUFFER_SIZE>
 where
-    TX: TxChannel,
+    TX: TxChannelAsync,
 {
     /// Create a new adapter object that drives the pin using the RMT channel.
     pub fn new<C, O>(
@@ -93,7 +93,7 @@ where
     ) -> SmartLedsAdapter<TX, BUFFER_SIZE>
     where
         O: OutputPin + 'd,
-        C: TxChannelCreator<'d, TX, O>,
+        C: TxChannelCreatorAsync<'d, TX, O>,
     {
         let config = TxChannelConfig {
             clk_divider: 1,
@@ -160,7 +160,7 @@ where

 impl<TX, const BUFFER_SIZE: usize> SmartLedsWrite for SmartLedsAdapter<TX, BUFFER_SIZE>
 where
-    TX: TxChannel,
+    TX: TxChannelAsync,
 {
     type Error = LedAdapterError;
     type Color = RGB8;
@@ -168,7 +168,7 @@ where
     /// Convert all RGB8 items of the iterator to the RMT format and
     /// add them to internal buffer, then start a singular RMT operation
     /// based on that buffer.
-    fn write<T, I>(&mut self, iterator: T) -> Result<(), Self::Error>
+    async fn write<T, I>(&mut self, iterator: T) -> Result<(), Self::Error>
     where
         T: IntoIterator<Item = I>,
         I: Into<Self::Color>,
@@ -187,14 +187,15 @@ where
         *seq_iter.next().ok_or(LedAdapterError::BufferSizeExceeded)? = 0;

         // Perform the actual RMT operation. We use the u32 values here right away.
-        let channel = self.channel.take().unwrap();
-        match channel.transmit(&self.rmt_buffer).wait() {
-            Ok(chan) => {
-                self.channel = Some(chan);
+        let mut channel = self.channel.take().unwrap();
+        match channel.transmit(&self.rmt_buffer).await {
+            Ok(_) => {
+                //self.channel = Some(chan);
                 Ok(())
             }
-            Err((e, chan)) => {
-                self.channel = Some(chan);
+            //Err((e, chan)) => {
+            Err(e) => {
+                //self.channel = Some(chan);
                 Err(LedAdapterError::TransmissionError(e))
             }
         }

Then adding async on the fn write trait member from smart-leds-trait crate, effectively having a breaking version of the (now async) trait, leveraging the relatively new (and simpler!) support for async traits in nightly.

[brainstorm]

Oh, scratch that comment about what to convert to async @Dominaezzz: I just realised that smart-leds-trait only implements TX because it only has the SmartLedsWrite trait definition... and what I commented about the photosensors (RX case) would be implemented via the already fully async RMT impl 🤦🏻

With this out of the way, it seems like I just need to carefully gate my changes through #[cfg(feature = "async")], so it might not be too tricky after all.