RMT driver tracking issue

[wisp3rwind]

Let's keep track of desirable refactorings and enhancements of the RMT driver. A bunch of these have come up in recent PRs, where it was decided to not address them immediately:

• Always type-erase the RMT channel number on Channel

  • mentioned in RMT: Refactor driver internals (less macros, type-erased channels) #3505 (comment)
  • PR: RMT: Type-erase channels #3980

• Move channel specification from cfg_if! branches to esp-metadata

  • came up in RMT: Some internal refactorings #3917 (comment)
  • PR: RMT: move channel specification to esp-metadata #4131
  • PR: RMT: move more things to esp-metadata, improve HIL test robustness #4138

• Consider redesigning the extended channel RAM feature to avoid global shared mutable state

  • cf. RMT: Some internal refactorings #3917 (comment)

• Support wrapping rx/tx for all methods

  • PR (partial): RMT: refactor copies to/from channel RAM #4126
  • PR: RMT: Support wrapping everywhere #4049

• Support more general data types (iterator, an encoder type similar to IDF)

  • also remove From<PulseCode>/Into<PulseCode> support: RMT: refactor copies to/from channel RAM #4126 (review)
  • PR (partial): RMT: refactor copies to/from channel RAM #4126
  • PR: ...
  • @wisp3rwind currently working on this

• Support multi-channel sync tx

• Implement blocking methods for owned and borrowed channels and redesign how data is passed in

  • came up in RMT: Refactor state checks, support non-blocking poll on blocking transactions #3716 (comment)
  • related: RMT API no wait #1749
  • PR (partial): RMT: lifetimes, tests, fixes #4174
  • @wisp3rwind currently working on this

• Properly tie Rmt, ChannelCreator, Channel lifetimes together.

  • PR: RMT: lifetimes, tests, fixes #4174

• Fractional divider support:

  • [RMT] Feature request: fractional divider support #2119

• Better reflect hardware capabilities for continuous tx in the API and add HIL tests:

  • came up in RMT: Misc refactoring #4100 (comment)
  • PR: RMT: clarify continuous tx API and add HIL tests #4260
  • PR: RMT: Obtain field witdths from PAC instead of esp-metadata, verify loopcount #4276
  • still missing: clarify/improve is_loopcount_interrupt_set()

• Add an async continuous tx method

  • @wisp3rwind curently working on this

• Ensure that all fallible methods that take Channel by value return it on error

  • PR: RMT: Various cleanups, small API enhancements #4302

I've marked points that I'm currently working on/have some local prototype for.

[kleinesfilmroellchen]

Does the point about lifetimes also mean fixing the current problem where you can’t configure the channel more than once, and if you start a transfer and it errors out during the initial configuration you lose safe access to the channel? Both of those could be fixed by just properly taking the parent structure by mutable reference on creation, which frees them up again after an error or a channel access ended.

[kleinesfilmroellchen]

For reference, here’s an abomination I had to write to emulate a temporarily borrowed transmit channel.

/// Better interface for borrowing RMT channels temporarily and giving them back after use, able to be reconfigured.
struct BorrowedRmtChannel<'d, const CHANNEL: u8, Int>
where
	Int: ChannelInternal,
{
	// Should usually contain a value, but because of lower-level API problems we have to empty it out once in a while.
	channel: Option<Channel<Blocking, Int>>,
	config:  TxChannelConfig,
	pin:     AnyPin<'d>,
	creator: &'d mut ChannelCreator<Blocking, CHANNEL>,
}

impl<'d> BorrowedRmtChannel<'d, 0, ConstChannelAccess<Tx, 0>> {
	pub fn configure_tx(
		creator: &'d mut ChannelCreator<Blocking, 0>,
		pin: AnyPin<'d>,
		config: TxChannelConfig,
	) -> Result<Self, Error> {
		// SAFETY: We borrow mutably from the channel creator, ensuring that noone else can use it for our lifetime.
		//         The lifetime of the channel is tied to us.
		let actual_channel = unsafe { ChannelCreator::<Blocking, 0>::steal() };
		// SAFETY: Noone except us has a copy of the pin. We only use our second copy in case the channel gets dropped.
		let configured = actual_channel.configure_tx(unsafe { pin.clone_unchecked() }, config)?;
		Ok(Self { channel: Some(configured), config, creator, pin })
	}

	/// Transmit a certain number of loops of the given number.
	pub async fn transmit_loops(&mut self, data: &[u32], loops: u16) -> Result<(), Error> {
		debug_assert!(self.channel.is_some());
		let channel = self.channel.take().unwrap();
		let res = channel.transmit_continuously_with_loopcount(loops, data).map_err(|e| {
			// SAFETY: This is a borrow checker workaround. All of this code exits the function. The only problems would
			// happen if this code executed after the map_err below, which it never can.
			*self = Self::configure_tx(
				unsafe { mem::transmute(self.creator as *mut _) },
				unsafe { self.pin.clone_unchecked() },
				self.config,
			)
			.unwrap();
			e
		})?;
		while !res.is_loopcount_interrupt_set() {
			Timer::after(Duration::from_millis(1)).await;
		}
		let channel = res.stop_next().map_err(|(e, channel)| {
			self.channel = Some(channel);
			e
		})?;
		self.channel = Some(channel);
		Ok(())
	}
}

[bugadani]

You can do Rmt::new(peripherals.RMT.reborrow(), FREQ) and that way you can reclaim the peripheral singleton once you're done with Rmt. On the other hand, I think this is currently unsound, ChannelCreator needs to have a lifetime otherwise it's possible to drop Rmt while channels/channel creator structs are alive, which essentially lets you create any number of copies of the same channel...

[kleinesfilmroellchen]

You can do Rmt::new(peripherals.RMT.reborrow(), FREQ) and that way you can reclaim the peripheral singleton once you're done with Rmt.

I need to have two different channels that do their own thing, so I’m not sure this will work, but it’s nice to know that the RMT peripheral can be reborrowed like the GPIOs (and others).

ChannelCreator needs to have a lifetime otherwise it's possible to drop Rmt while channels/channel creator structs are alive, which essentially lets you create any number of copies of the same channel...

Yeah, that’s true. Would be easy enough to add, but doesn’t that mean that you can do unsound things with the upstream API without unsafe since ChannelCreator is not my invention?

[bugadani]

but doesn’t that mean that you can do unsound things with the upstream API without unsafe since ChannelCreator is not my invention?

Absolutely, this is a problem that exists right now.

[kleinesfilmroellchen]

Looking at it again, though, I don’t know what you’re exactly referring to in my code. Since I take ChannelCreator by mutable reference, as long as ChannelCreator depends on Rmt to be alive, things are fine. For me it would also just be adding another lifetime argument in the code above if that were to be changed. But either way, I’d rather want some API upstream that does what I’m hacking in here automatically, and I wanted to give an idea of what kind of API I need in my code. To improve this idea further, it’s probably useful to show the code that uses it as well:

async fn test_rmt(rmt_peripheral: peripherals::RMT<'_>, mut target: AnyPin<'_>) -> Result<(), esp_hal::rmt::Error> {
	// adjust clock to specific chip, here 80MHz on ESP32-C6
	let mut rmt = Rmt::new(rmt_peripheral, RMT_BASE_CLOCK)?;

	loop {
		// not important -- calculate the RMT config and pulse data based on some complex logic
		let config = rmt_data_for_steps(RMT_BASE_CLOCK, steps, time).unwrap();
		let target = target.reborrow();
		let mut channel = BorrowedRmtChannel::configure_tx(&mut rmt.channel0, target.into(), (&config).into())?;
		channel.transmit_loops(&config.pulses, config.loops).await?;
		Timer::after(Duration::from_millis(1000)).await;
	}
}

[wisp3rwind]

Does the point about lifetimes also mean fixing the current problem where you can’t configure the channel more than once,

yes

and if you start a transfer and it errors out during the initial configuration you lose safe access to the channel?

yes

Both of those could be fixed by just properly taking the parent structure by mutable reference on creation, which frees them up again after an error or a channel access ended.

not quite, since it is desirable to have an API that takes the channel by value: #3716 (comment)

---

You can do Rmt::new(peripherals.RMT.reborrow(), FREQ) and that way you can reclaim the peripheral singleton once you're done with Rmt.

That works right now due to the issue you point out below, but it's not a viable solution when using several channels independently.

On the other hand, I think this is currently unsound, ChannelCreator needs to have a lifetime otherwise it's possible to drop Rmt while channels/channel creator structs are alive, which essentially lets you create any number of copies of the same channel...

In fact what you suggest doesn't quite work out, but only by accident since #3453: It does compile, but due to tracking some global state it will error out when trying to configure the channel before the first instance is dropped.

---

Yeah, that’s true. Would be easy enough to add, but doesn’t that mean that you can do unsound things with the upstream API without unsafe since ChannelCreator is not my invention?

Currently, yes, but see the above comment on runtime errors.

---

The (probably) next PR I intend to submit after #4138 will address the above points. It should then be possible to do something along the lines of (note the .reborrow() on the ChannelCreator)

async fn test_rmt(rmt_peripheral: peripherals::RMT<'_>, mut target: AnyPin<'_>) -> Result<(), esp_hal::rmt::Error> {
    // adjust clock to specific chip, here 80MHz on ESP32-C6
    let mut rmt = Rmt::new(rmt_peripheral, RMT_BASE_CLOCK)?;

    loop {
        let config = rmt_data_for_steps(RMT_BASE_CLOCK, steps, time).unwrap();
        let channel = rmt.channel0
            .reborrow()
            .configure_tx(target.reborrow(), (&config).into())?;
        let transaction = channel
            .transmit_continuously_with_loopcount(&config.pulses, config.loops)
            .await?;
        while !transaction.is_loopcount_interrupt_set() {}
        Timer::after(Duration::from_millis(1000)).await;
    }
}

That PR (mostly ready locally) should enable most use cases that currently don't work, it might not be the most ergonomic for all of those yet.

---

As an aside; does submit_continuously_with_loopcount actually work for you? I've been writing a test, and concluded that it's actually mostly broken (esp32c6 being one of the devices where it's reasonably easy to fix, at least).

[wisp3rwind]

		// SAFETY: We borrow mutably from the channel creator, ensuring that noone else can use it for our lifetime.
		//         The lifetime of the channel is tied to us.
		let actual_channel = unsafe { ChannelCreator::<Blocking, 0>::steal() };

This is enough for safety regarding channel duplication, but there's another problem with current RMT API here: If before the ChannelCreator::steal() all Channel, ChannelCreators and the Rmt have been dropped the peripheral clock will be stopped and re-enabled, which loses the clock configuration.

[kleinesfilmroellchen]

@wisp3rwind thanks for the reply, and I’m happy that you’re planning to fix pretty much every problem I’m having with the current API. Your rewrite of my existing code looks very nice and would definitely be a workable solution for my and similar use-cases.

As an aside; does submit_continuously_with_loopcount actually work for you? I've been writing a test, and concluded that it's actually mostly broken (esp32c6 being one of the devices where it's reasonably easy to fix, at least).

I’m fairly certain it does. I don’t have access to an oscilloscope until the weekend, so I am not sure how well exactly it works for higher transmission speeds, but I know that the total transmission time seems to be (almost) correct (e.g. when I use a combination of clock dividers, pulse lengths and loop counts that come out to exactly 10s of transmission time, and hook up an LED to the RMT output, I can see that it lights up with the RMT “PWM” modulation for pretty much exactly 10s). This is true for a variety of combinations of loop counts, clock dividers, and number of pulses. It even works for extreme values, such as exactly two pulses over a 10s period with a super high divider and pulse length, where I can see and approximately time each pulse. So I don’t think there are any major issues with functionality apart from missing the fractional divider value, which you stated you want to get working eventually and which is not a necessary feature for my use case anyways.