Allow a way to get >1 `impl Delay` type

[tgross35]

It seems like at this time, it is only possible to get one Delay : atsamd_hal::delay::Delay, which consumes the systick. I brought this issue up on the embedded HAL crate, and their suggestion was that there should be a way to get more than one delay viaClone` or otherwise. https://github.com/rust-embedded/embedded-hal/issues/435

I'm not sure what the best way to go about this is, but it should be possible via this HAL. Being able to get more impl CountDown objects would be similarly helpful.

[bradleyharden]

@tgross35, yes, that seems like a reasonable feature. Feel free to make a PR. I'm not really familiar with the Delay API, but other maintainers should be.

[tgross35]

The inner workings aren't too complicated, but I am really not sure what the best way forward is that also avoids race conditions. This chunk specifically definitely seems problematic https://github.com/atsamd-rs/atsamd/blob/7d85efd1f0c8ce136a9bdcb96cc4d0fe672bed6b/hal/src/delay.rs#L65-L67

I don't mind doing the work, but I would appreciate some input from the proper maintainers if you know who to ping

[Sympatron]

A shallow google search seems to indicate that most (at least all I could find) Delay implementations work basically the same. They set the match register, reset the timer and wait synchronously (in a busy loop) for a match. No implementations I could find tries to be cloneable.

The only way I can see it work is by wrapping the Delay in a Mutex. To make it compatible with APIs requiring a Delay object, one could perhaps implement Delay for Mutex<Delay> (or some kind of wrapper around that to avoid orphan rules).

That being said, I am no expert in Rust, so maybe there is another way.

[tgross35]

A mutex or RefCell is what jumped into my mind as well, but it doesn't seem like a good idea to add that overhead when some delays may be very short.

I am curious what Adam comes up with for cortex M, as mentioned here https://github.com/rust-embedded/embedded-hal/issues/435#issuecomment-1411070489

edit: scratch my ISR example since I just realized the peripheral would have to be in a mutex anyway

[tgross35]

I think you could do things properly if the current Delay is used and allowed to Clone (with some interior mutability on SYST), but it would need to be !Send to make sure you couldn’t send it between threads or e.g. to an interrupt handler. But this would be a breaking change

[Sympatron]

If it's !Send anyway your should be able to get away with a RefCell because it can't be accessed concurrently in that case. I think that would work, but then you definitely cannot use it in ISRs without a Mutex.

[tgross35]

Qutoing from Adam's suggestion in https://github.com/rust-embedded/embedded-hal/issues/435#issuecomment-1439195657:

3. A struct that consumes SysTick (maybe with a release() method), configures it to run at some known rate, and then allows you to create as many new Delay objects as you want which only read the systick counter to work out how long has passed.

... For a HAL, something like option 3 but using an actual hardware timer is best, I think. It "uses up" the timer to ensure it's always running at a known frequency, then each of the Delays just watches until enough counts have passed on the timer (handling wraparound as required).

I would assume that changing the current Delay API is not acceptable, so perhaps something like this would work:

• Delay struct keeps its current external API
• Delay also allows a way to get a BorrowedDelay or something that takes an & to the original delay (thus in this case, forbidding Delay::delay_ms(&mut self)).
• Delay should keep SYST within an UnsafeCell to allow for interior mutability. It should provide a private API for the BorrowedDelays to get a simple counter of the systick plus something a wrap counter that they could compare to their timeouts
• BorrowedDelay would implement embedded_hal::Delay just using this API, rathter than going via the systick reload register

That way, there would only ever be one instance of Delay that interacts with the systick so worries about Send go away.

Alternatively, an entirely separate set of types like SharedDelay and BorrowedDelay could be introduced, or change the current Delay API.

Does any of this sound reasonable?

[jbeaurivage]

@tgross35, another approach would be using a timer queue. When scheduling timeouts, they are added to a queue, and the timer can then generate an event when the next timeout expires. This enables the timer to be Send + Sync and even be used using an API based on shared references only. RTIC has an utility crate that does exactly this (rtic-monotonics), though it's based on an async API. We could hopefully leverage this instead of rolling our own!

PS. there are ways to use async constructs in a sync program using simple, lightweight executors such as spin_on.

[ianrrees]

I wonder if the right answer is to change things that require Delay to instead use embedded-hal's DelayNs (then, perhaps rename our Delay to something clearer, like SystickDelay).

Currently, Delay implements DelayNs, as do the timer/counters. It would probably not be too hard to implement some of the above suggestions on top of DelayNs, in whatever way is suitable for the situation at hand.

[ianrrees]

Sorry, I should've spent 5 minutes looking in to this before commenting... All our uses of Delay that I looked at would be trivial to replace with any DelayNs impl, so for cases that need a small number of delays it's probably reasonable to use the existing systick and timer/counter support (edit: once we've got it - this doesn't seem to be in the HAL currently) to get multiple delays.

Making an interruptible delay, that's also reasonably accurate, I think would require some way to access the state of the hardware counter, so would need to be in the HAL for a TC or TCC implementation. Doing something like a loop around a 1us delay from an arbitrary impl DelayNs could work, but I think would have to choose between some pretty bad options like poor accuracy (it seems inevitable that it would accumulate error), or sometimes getting a substantially longer delay than requested (would have to wait for the delay in the loop to finish).

Seems like a fun little puzzle; I think for me usb-device issues and clocking v2 implementation for the thumbv6 targets are both higher priority at the moment but will have a think on this one.

[ianrrees]

Apologies for another reply-to-self... The SYSTICK is provided by the processor core, so we might look around at other Cortex-M HALs to see what's available.

It wouldn't be hard to make a struct that crudely impls DelayNs using cortex_m::asm::delay - I wouldn't be surprised if someone's done this already but I've had a quick look around and not found any options.

I reckon it would be possible to make a Send and Clone DelayNs implementation where the SYSTICK is owned in a 'static struct that also has some bookkeeping to keep track of when delays interrupt each other. At the start of a delay, code in an atomic block would read the state of the SYSTICK and any interrupted delays, before modifying SYSTICK as needed. Once that's done it would restore the state of the SYSTICK less the amount of time it used, and update the state of the interrupted delays. It's not as simple as just updating SYSTICK, because if you had delay_ms(10), A, interrupted by delay_ms(1), B, interrupted by delay_ms(10), C, you'd want A and B to expire immediately after C. In that case, the naive implementation (modifying only SYSTICK) would have C's expiry causing B to expire immediately, but then B's expiry could only subtract 1ms off A, leaving it to continue on for up to 9 further ms.