Circular Transfers from ReadyFuture Channels.

[QuartzShard]

Summary

I ran into an issue where configuring my DMAC in future-mode (for use with async SERCOM) prevented me from creating a circular transfer for the PCC. I need both, so I resorted to the sketchy approach of transmuting the ReadyFuture channel back into a Ready one, which as far as I can tell, is Fine as long as I only start circular transfers on that channel, so the TCMPL interrupt never fires and the handler ignores this channel.

This PR is an attempt to permit this behavior without transmute by allowing the creation of circular transfers specifically with ReadyFuture channels, and disallowing the creation of linear transfers by constraining the type back to just Ready on the existing constructors, instead of bound by ReadyChannel - closing the gap that would permit a linear transfer to start and be handled by the interrupt when it wasn't started as a TransferFuture.

Checklist

• All new or modified code is well documented, especially public items
• No new warnings or clippy suggestions have been introduced - CI will deny clippy warnings by default! You may #[allow] certain lints where reasonable, but ideally justify those with a short comment.

[jbeaurivage]

Hey @QuartzShard, I'm trying to understand the usecase for this a little better. If your SERCOM is using circular transfers, is there a need to use the async implementation in the first place? As far as I can tell, the transfer would actually never complete until it is cancelled.

Unless I'm missing something, .awaiting the transfer doesn't seem to have an advantage over starting a (non-blocking) transfer?

[QuartzShard]

The SERCOM is using non-circular transfers with the async api, I have a DVP camera connected to the PCC for which I'm using circular. The current API doesn't allow these to coexist as far as I can tell, even though there's no hardware reason to disallow this.

[QuartzShard]

For context, this is what I've ended up doing in my application to get around this, which I'm pretty sure is valid for ReadyFuture channels as long as whatever I do with them afterwards doesn't fire a TCMPL:

pub fn new<R: ReadyChannel>(channel: Channel<C, R>, source: S, dest: D) -> Self {
    assert!(
        source.buffer_len() == dest.buffer_len()
            || source.buffer_len() == 1
            || dest.buffer_len() == 1
    );
    // SAFETY: We just asserted that the Bufferpair is valid - and the Drop impl ensures the
    // transfer is stopped before the `SafeTransfer` is destroyed
    let transfer = unsafe {
        // SAFETY: This is valid as the only difference is PhantomData, so long as we don't
        // break contract by enabling the TCMPL interrupt
        let channel: Channel<C, Ready> = core::mem::transmute(channel);
        Transfer::new_unchecked(channel, source, dest, true)
            .begin(TriggerSource::PccRx, TriggerAction::Burst) // This does not compile if R is ReadyFuture
    };
    Self {
        inner: ManuallyDrop::new(transfer),
    }
}

[jbeaurivage]

@QuartzShard thanks for this PR. I've been meaning to look at it for a bit and finally managed find some time. This looks pretty good, still need to go a little more in-depth, but I had one thought that I wanted to write down nonetheless.

A bit of a parallel issue is that without a way to transform an async-enabled Channel back into a non-async one, once a user opts into the async DMAC, they are essentially locked out of using the blocking methods since the whole DMAC peripheral shares one ISR. I'm not sure that is solvable without some major re-architecting though.

[QuartzShard]

@jbeaurivage I had that same thought, but opted to not try and implement that, since the shared ISR presents a safety issue if TCMPL is enabled for a non-async channel. If it could be enforced that TCMPL could not be enabled for a blocking channel then I think it'd be fine to let users "Downcast" into a blocking channel freely, but like you said, it'd require some work.

Maybe, as a stop-gap, we could provide an unsafe function to do this, and add the "No TCMPL" as part of its' invariants?

[jbeaurivage]

IMO adding a pathway to convert an async channel into a blocking channel seems to be easiest and lowest friction way to go about it. It also removes the need to add a special case for circular transfers, since I don't think it really makes sense to do circular transfers on async channels.

Actually now that I think about it, it wouldn't even be unsafe to enable the TCMPL interrupt for a blocking-but-used-to-be-async channel. It's certainly incorrect, and these things will happen:

• The DMAC ISR will run
• The TCMPL interrupt will be disabled, but the flag won't be cleared
• The channel will potentially get disabled
• The corresponding waker will potentially get woken

That being said, we're using embassy_sync::waitqueue::AtomicWaker, which internally will only wake the executor if a waker had previously been registered, and that is only possible to do with an async channel. It should definitely be documented correctly though. Also I don't think there'd be a way to convert an async channel into a sync one while a waker had been registered but the Future hasn't completed yet, so there shouldn't have no memory unsafety here. (You would have to poll it and convert the channel into a sync channel before the Future completes, but that's impossible because the Channel is mutably borrowed into the returned Future).

What do you think?

[jbeaurivage]

I just thought about this: Perhaps the Channel could hold some third generic type parameter that holds a certain type before it gets converted to an async Channel, and we only implement the interrupt enable/disable methods for this specific type. Then when it gets converted into an async channel, that type param switches with no way to return to the previous type state. I think that should be fine, since it makes no sense to try to enable/disable the interrupts for an async channel anyway, the interrupt enable flag gets taken over as soon as you start a transfer.

[QuartzShard]

I'll be honest, I'm not experienced with the "guts" of async implementations at all, so as soon as I saw the Waker management in the ISR I sort of just assumed that "this way be unsoundness" and settled in the assumption that "triggering a TCMPL on an async channel would be unsound". As you said though, even if it's not necessarily unsafe, it's incorrect.
As for adding a new Type param, I think this is the best (if not the highest-effort) way to actually permit the widest array of correct behaviour through the HAL. That would give us a new Typelevel enum, let's call it "InterruptAvailable", with states "Available" and "Blocked". Channels you get from a blocking-mode controller are "Available", so can have interrupt setting methods called, and Channels you get from an async-mode controller are "Blocked". This lets us freely convert an Async channel into a Blocking channel, so long as "Blocking" is preserved?
I'd be happy to implement that instead of the Special-Case conversion I've done here, for sure - and we can set the default to "Available" to not upset existing code as much. Sound good to you?

[jbeaurivage]

I'd be happy to implement that instead of the Special-Case conversion I've done here, for sure - and we can set the default to "Available" to not upset existing code as much. Sound good to you?

Absolutely, please feel free to go ahead with this. If you don't mind, it would also be nice to document these new features.

One thing you'll likely have to do is have private interrupt enable/disable methods that aren't type-constrained, so that we can privately call them in the async API methods, and their pub counterparts implemented only on the Available type parameter.

[QuartzShard]

Obsoleted by #988