RMT: Type-erase channels (#3980)

* RMT: add HIL test that exercises all channels

to ensure that there are no issues with channel/register indexing; see
also the code comments.

no user-facing changes

* RMT: store ch_idx instead of channel number

`ConstChannelAccess` and `DynChannelAccess` used to store the channel number
and have a `Direction` generic parameter.
However, there are invalid combinations of `Direction` and the channel
number on some chips (e.g. on esp32c3, there is no channel with (Tx, 2),
because channel 2 and 3 only support Rx).

In constrast, the tuple `(Dir, ch_idx)` also uniquely identifies a
channel and its configuration, and any combination of it is valid as long as the
channel index is in bounds.

This makes it somewhat easier to work with; in particular, it will allow
removing bounds checks on ch_idx in a later commit by replacing
`ch_idx: u8` with an enum type.

This also refactors the `async_interrupt_handler` accordingly.

This is user-visible via the `ConstChannelAccess` type, however a later
commit in this PR will remove that entirely.

* RMT: elide bounds checks via ChannelIndex enum

If the channel index is not known at compile time in a given function,
the compiler will insert bounds checks on

- indexing the STATE global
- register access via the PAC

The compiler seems to be able to deduplicate them since low-level
functions are mostly inlined, but each function will retain at least one
bounds check.

This change helps the compiler to elide these checks by using a custom
type for channel indices that can only take values for which a channel
exists.
Specifically, this uses an enum to serve as a refinement of u8 to help
the compiler restrict value ranges. Due to inlining, this restricted
value range is propagated by the compiler until the potential bounds checks,
even if there's intermediate cast to u8,
cf. https://github.com/rust-lang/rust/issues/109958

There are no user-facing changes. (`ChannelIndex` is `pub` because it
appears in a `[doc(hidden)]` trait member.)

* RMT: always type-erase channels

- remove the `Raw` generic on `Channel`: This simplifies the types,
  should have negligible performance impact (helped by elided bounds
  checks via the recently introduced `ChannelIndex` enum), and greatly
  reduce code size if several channels are used (by avoiding
  monomorphization per channel)

- this also changed the arguments to configure_(tx,rx)_channel such that
  they don't contain generics, again avoiding code bloat due to
  monomorphization

This requires user code to change type annotations for channels and
transactions, and remove any manual type erasure (via `channel.degrade()`).

* RMT: impl blocking tx/rx methods directly on Channel

There's now only a single type that should implement these methods due
to erasing the channel index const generic, so the indirection via a
trait serves no purpose. Implementing methods directly on the channel
probably also helps to make the docs more discoverable.

This requires user code to remove the `TxChannel` and `RxChannel`
imports.

* RMT: impl async tx/rx methods directly on Channel

There's now only a single type that should implement these methods due
to erasing the channel index const generic, so the indirection via a
trait serves no purpose. Implementing methods directly on the channel
probably also helps to make the docs more discoverable.

This requires user code to remove the `TxChannelAsync` and `RxChannelAsync`
imports.

* RMT: impl *ChannelInternal traits only on DynChannelAccess

rather than as a blanket impl for RawChannelAccess, which includes
ConstChannelAccess: We don't intend to perform any accesses via
ConstChannelAccess anymore, so enforce that.
We could also get rid of the traits entirely and directly impl their
methods for DynChannelAccess, but it seems somewhat useful to keep them
around to guarantee that the interfaces in both chip_specific modules
are consistent.

There are no user-facing changes here.

* RMT: remove `pub` on various internal types

that used to be visible somewhere in the API's trait bounds, but are not
part of the API themselves

This is user-visible, but user code should not have used these types in
the first place.

* RMT: rm ConstChannelAccess, RawChannelAccess, private DynChannelAccess

now that Channel has no `Raw: RawChannelAccess` type parameter:

- we don't need low-level channel methods to be implemented for
  ConstChannelAccess, since all accesses go through DynChannelAccess.
  Thus, remove the *ChannelInternal traits and implement their methods directly
  on DynChannelAccess
- none of these types need to be public (although one might consider
  actually making them public with an unsafe constructor to provide an
  unsafe low-level interface to the hardware)

This is user-visible (imports need to be removed; they're not used
anymore since the previous commit that always type-erased the `Raw`
parameter of `Channel`)

* RMT: changelog and migration guide for type-erased channels

* RMT: (review) remove stray comment

* RMT: (review) remove is_tx() from Direction trait

we can simply use the const item directly

esp-hal/CHANGELOG.md

@@ -51,6 +51,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - ESP32-S3: `PsramConfig::core_clock` is now an `Option` (#3974)
 - `RtcSlowClock::RtcFastClock8m` has been renamed to `RtcFastClock::RtcFastClockRcFast` (#3993)
 - `RtcSlowClock::RtcSlowClockRtc` has been renamed to `RtcSlowClock::RtcSlowClockRcSlow` (#3993)
+- The `Raw: RawChannelAccess` of `rmt::Channel` has been erased; channel numbers are always dynamic now. (#3980)
 
 ### Fixed
 

esp-hal/MIGRATING-1.0.0-rc.0.md

@@ -79,7 +79,7 @@ esp_hal::interrupt::bind_interrupt(
 );
 ```
 
-## RMT changes
+## RMT PulseCode changes
 
 `PulseCode` used to be an extension trait implemented on `u32`. It is now a
 newtype struct, wrapping `u32`.
@@ -113,6 +113,42 @@ let _ = tx_channel.transmit(&tx_data).wait().unwrap();
 let _ = rx_channel.transmit(&mut rx_data).wait().unwrap();
 ```
 
+## RMT Channel Changes
+
+`rmt::Channel` used to have a `Raw: RawChannelAccess` generic parameter,
+which could be either `ConstChannelAccess<Dir, const CHANNEL: u8>` or `DynChannelAccess<Dir>`.
+This generic has been erased, effectively always using `DynChannelAccess`.
+The corresponding parameter of the transaction structs has been removed as well
+(`SingleShotTxTransaction`, `ContinuousTxTransaction`, `RxTransaction`).
+
+Transmit and receive methods are now directly implemented by
+`Channel<Dm: DriverMode, Tx>`
+and
+`Channel<Dm: DriverMode, Rx>`
+respectively and the `RxChannel`, `TxChannel`, `RxChannelAsync` and `TxChannelAsync`
+traits have been removed.
+Several related types that were previously exported have been removed from the
+API as well.
+
+```diff
+-use esp_hal::rmt::{ConstChannelAccess, DynChannelAccess, RawChannelAccess};
+-let mut tx: Channel<Blocking, ConstChannelAccess<Tx, 0>> = rmt.channel0.configure_tx(NoPin, TxChannelConfig::default());
+-let mut rx: Channel<Blocking, ConstChannelAccess<Rx, 2>> = rmt.channel2.configure_rx(NoPin, RxChannelConfig::default());
++let mut tx: Channel<Blocking, Tx> = rmt.channel0.configure_tx(NoPin, TxChannelConfig::default());
++let mut rx: Channel<Blocking, Rx> = rmt.channel2.configure_rx(NoPin, RxChannelConfig::default());
+
+-let mut tx: Channel<Blocking, DynChannelAccess<Tx>> = tx.degrade();
+-let mut rx: Channel<Blocking, DynChannelAccess<Rx>> = rx.degrade();
+
+-// same for TxChannelAsync, RxChannelAsync
+-use esp_hal::rmt::{TxChannel, RxChannel};
+-
+-let tx_transaction: SingleShotTxTransaction<'_, DynChannelAccess<Tx>, PulseCode> = tx.transmit(&data);
+-let rx_transaction: RxTransaction<'_, DynChannelAccess<Rx>, PulseCode> = rx.transmit(&data);
++let tx_transaction: SingleShotTxTransaction<'_, PulseCode> = tx.transmit(&data);
++let rx_transaction: RxTransaction<'_, PulseCode> = rx.transmit(&data);
+```
+
 ## DMA changes
 
 DMA buffers now have a `Final` associated type parameter. For the publicly available buffer, this is `Self`,
@@ -184,4 +220,4 @@ let peripherals = esp_hal::init(
 + .with_data5(peripherals.GPIO17)
 + .with_data6(peripherals.GPIO16)
 + .with_data7(peripherals.GPIO15);
-```
+```

examples/async/embassy_rmt_rx/src/main.rs

@@ -11,7 +11,7 @@ use embassy_time::{Duration, Timer};
 use esp_backtrace as _;
 use esp_hal::{
     gpio::{Level, Output, OutputConfig},
-    rmt::{PulseCode, Rmt, RxChannelAsync, RxChannelConfig, RxChannelCreator},
+    rmt::{PulseCode, Rmt, RxChannelConfig, RxChannelCreator},
     time::Rate,
     timer::timg::TimerGroup,
 };

examples/async/embassy_rmt_tx/src/main.rs

@@ -13,7 +13,7 @@ use embassy_time::{Duration, Timer};
 use esp_backtrace as _;
 use esp_hal::{
     gpio::Level,
-    rmt::{PulseCode, Rmt, TxChannelAsync, TxChannelConfig, TxChannelCreator},
+    rmt::{PulseCode, Rmt, TxChannelConfig, TxChannelCreator},
     time::Rate,
     timer::timg::TimerGroup,
 };

hil-test/tests/rmt.rs

@@ -7,16 +7,18 @@
 #![no_main]
 
 use esp_hal::{
+    Blocking,
     DriverMode,
     gpio::{InputPin, Level, NoPin, OutputPin},
     rmt::{
-        AnyRxChannel,
-        AnyTxChannel,
+        Channel,
         Error,
         PulseCode,
         Rmt,
+        Rx,
         RxChannelConfig,
         RxChannelCreator,
+        Tx,
         TxChannelConfig,
         TxChannelCreator,
     },
@@ -40,12 +42,11 @@ fn setup<Dm: DriverMode>(
     tx: impl OutputPin,
     tx_config: TxChannelConfig,
     rx_config: RxChannelConfig,
-) -> (AnyTxChannel<Dm>, AnyRxChannel<Dm>) {
+) -> (Channel<Dm, Tx>, Channel<Dm, Rx>) {
     let tx_channel = rmt
         .channel0
         .configure_tx(tx, tx_config.with_clk_divider(DIV))
-        .unwrap()
-        .degrade();
+        .unwrap();
 
     cfg_if::cfg_if! {
         if #[cfg(any(esp32, esp32s3))] {
@@ -56,8 +57,7 @@ fn setup<Dm: DriverMode>(
     };
     let rx_channel = rx_channel_creator
         .configure_rx(rx, rx_config.with_clk_divider(DIV))
-        .unwrap()
-        .degrade();
+        .unwrap();
 
     (tx_channel, rx_channel)
 }
@@ -75,22 +75,10 @@ fn generate_tx_data<const TX_LEN: usize>(write_end_marker: bool) -> [PulseCode;
     tx_data
 }
 
-// Run a test where some data is sent from one channel and looped back to
-// another one for receive, and verify that the data matches.
-fn do_rmt_loopback<const TX_LEN: usize>(tx_memsize: u8, rx_memsize: u8) {
-    use esp_hal::rmt::{RxChannel, TxChannel};
-
-    let peripherals = esp_hal::init(esp_hal::Config::default());
-    let (rx, tx) = hil_test::common_test_pins!(peripherals);
-    let rmt = Rmt::new(peripherals.RMT, FREQ).unwrap();
-
-    let tx_config = TxChannelConfig::default().with_memsize(tx_memsize);
-    let rx_config = RxChannelConfig::default()
-        .with_idle_threshold(1000)
-        .with_memsize(rx_memsize);
-
-    let (tx_channel, rx_channel) = setup(rmt, rx, tx, tx_config, rx_config);
-
+fn do_rmt_loopback_inner<const TX_LEN: usize>(
+    tx_channel: Channel<Blocking, Tx>,
+    rx_channel: Channel<Blocking, Rx>,
+) {
     let tx_data: [_; TX_LEN] = generate_tx_data(true);
     let mut rcv_data: [PulseCode; TX_LEN] = [PulseCode::default(); TX_LEN];
 
@@ -115,9 +103,24 @@ fn do_rmt_loopback<const TX_LEN: usize>(tx_memsize: u8, rx_memsize: u8) {
 
 // Run a test where some data is sent from one channel and looped back to
 // another one for receive, and verify that the data matches.
-async fn do_rmt_loopback_async<const TX_LEN: usize>(tx_memsize: u8, rx_memsize: u8) {
-    use esp_hal::rmt::{RxChannelAsync, TxChannelAsync};
+fn do_rmt_loopback<const TX_LEN: usize>(tx_memsize: u8, rx_memsize: u8) {
+    let peripherals = esp_hal::init(esp_hal::Config::default());
+    let (rx, tx) = hil_test::common_test_pins!(peripherals);
+    let rmt = Rmt::new(peripherals.RMT, FREQ).unwrap();
 
+    let tx_config = TxChannelConfig::default().with_memsize(tx_memsize);
+    let rx_config = RxChannelConfig::default()
+        .with_idle_threshold(1000)
+        .with_memsize(rx_memsize);
+
+    let (tx_channel, rx_channel) = setup(rmt, rx, tx, tx_config, rx_config);
+
+    do_rmt_loopback_inner::<TX_LEN>(tx_channel, rx_channel);
+}
+
+// Run a test where some data is sent from one channel and looped back to
+// another one for receive, and verify that the data matches.
+async fn do_rmt_loopback_async<const TX_LEN: usize>(tx_memsize: u8, rx_memsize: u8) {
     let peripherals = esp_hal::init(esp_hal::Config::default());
     let (rx, tx) = hil_test::common_test_pins!(peripherals);
     let rmt = Rmt::new(peripherals.RMT, FREQ).unwrap().into_async();
@@ -152,8 +155,6 @@ fn do_rmt_single_shot<const TX_LEN: usize>(
     tx_memsize: u8,
     write_end_marker: bool,
 ) -> Result<(), Error> {
-    use esp_hal::rmt::TxChannel;
-
     let peripherals = esp_hal::init(esp_hal::Config::default());
     let (rx, tx) = hil_test::common_test_pins!(peripherals);
     let rmt = Rmt::new(peripherals.RMT, FREQ).unwrap();
@@ -281,4 +282,92 @@ mod tests {
             .configure_tx(NoPin, TxChannelConfig::default())
             .unwrap();
     }
+
+    macro_rules! test_channel_pair {
+        (
+            $peripherals:ident,
+            $tx_pin:ident,
+            $rx_pin:ident,
+            $tx_channel:ident,
+            $rx_channel:ident
+        ) => {
+            // It's currently not possible to reborrow ChannelCreators and reconfigure channels, so
+            // we reconfigure the whole peripheral for each sub-test.
+            let rmt = Rmt::new($peripherals.RMT.reborrow(), FREQ).unwrap();
+
+            let tx_config = TxChannelConfig::default();
+            let rx_config = RxChannelConfig::default().with_idle_threshold(1000);
+
+            let tx_channel = rmt
+                .$tx_channel
+                .configure_tx($tx_pin.reborrow(), tx_config)
+                .unwrap();
+
+            let rx_channel = rmt
+                .$rx_channel
+                .configure_rx($rx_pin.reborrow(), rx_config)
+                .unwrap();
+
+            do_rmt_loopback_inner::<20>(tx_channel, rx_channel);
+        };
+    }
+
+    // A simple test that uses all channels: This is useful to verify that all channel/register
+    // indexing in the driver is correct. The other tests are prone to hide related issues due to
+    // using low channel numbers only (in particular 0 for the tx channel).
+    #[test]
+    fn rmt_use_all_channels() {
+        let mut p = esp_hal::init(esp_hal::Config::default());
+        let (mut rx, mut tx) = hil_test::common_test_pins!(p);
+
+        // FIXME: Find a way to implement these with less boilerplate and without a macro.
+        // Maybe use ChannelCreator::steal() (doesn't help right now because it uses a const
+        // generic channel number)?
+
+        // Chips with combined RxTx channels
+        #[cfg(esp32)]
+        {
+            test_channel_pair!(p, tx, rx, channel0, channel1);
+            test_channel_pair!(p, tx, rx, channel0, channel2);
+            test_channel_pair!(p, tx, rx, channel0, channel3);
+            test_channel_pair!(p, tx, rx, channel0, channel4);
+            test_channel_pair!(p, tx, rx, channel0, channel5);
+            test_channel_pair!(p, tx, rx, channel0, channel6);
+            test_channel_pair!(p, tx, rx, channel0, channel7);
+
+            test_channel_pair!(p, tx, rx, channel1, channel0);
+            test_channel_pair!(p, tx, rx, channel2, channel0);
+            test_channel_pair!(p, tx, rx, channel3, channel0);
+            test_channel_pair!(p, tx, rx, channel4, channel0);
+            test_channel_pair!(p, tx, rx, channel5, channel0);
+            test_channel_pair!(p, tx, rx, channel6, channel0);
+            test_channel_pair!(p, tx, rx, channel7, channel0);
+        }
+
+        #[cfg(esp32s2)]
+        {
+            test_channel_pair!(p, tx, rx, channel0, channel1);
+            test_channel_pair!(p, tx, rx, channel0, channel2);
+            test_channel_pair!(p, tx, rx, channel0, channel3);
+
+            test_channel_pair!(p, tx, rx, channel1, channel0);
+            test_channel_pair!(p, tx, rx, channel2, channel0);
+            test_channel_pair!(p, tx, rx, channel3, channel0);
+        }
+
+        // Chips with separate Rx and Tx channels
+        #[cfg(esp32s3)]
+        {
+            test_channel_pair!(p, tx, rx, channel0, channel4);
+            test_channel_pair!(p, tx, rx, channel1, channel5);
+            test_channel_pair!(p, tx, rx, channel2, channel6);
+            test_channel_pair!(p, tx, rx, channel3, channel7);
+        }
+
+        #[cfg(not(any(esp32, esp32s2, esp32s3)))]
+        {
+            test_channel_pair!(p, tx, rx, channel0, channel2);
+            test_channel_pair!(p, tx, rx, channel1, channel3);
+        }
+    }
 }