RFC: Introducing a common interface for counters.

[Ben-PH]

Initially intended as a Clock rfc, my thoughts evolved while writing. Though this could be returned to a simple Clock RFC, a clock interface is nothing more than

1. Reading a generic integer value that increments 1 at a time. For a clock, this is an oscilation count
2. A specification of a measurement assosciated with each increment. For a clock, this is a duration of time

There are many situations where these two principals are used, and they are distinct only in the what they are reading (u32, u64, u8, etc), and what they are measuring (1/80_000_000 seconds per count? 1 milimeter per count? 100 milliradians per count?).

I chose to generalise, so as to entrench the idea that this RFC aims to bring in something that incurs no restrictions on reverse dependencies, and portability limitation defined only by the inherent nature of clocks and other counters.

I anticipate a Counter trait emerging, with other more specific expressions of counting to emerge, such as a Clock: Counter and PulseCount: Counter traits, in such a way that the extensions add additional constraints (e.g. you're counting something that can be a measurement of time). The RFC file goes into detail.

A list of reasonable expectations proposed by @jamesmunns

• [ ] A timer/counter will need to be shared - there are relatively few of them on many chips
• [x] Users will want high precision in some cases, 1k-1M are very reasonable number
• [ ] Some timers have a limited range - 16/24/32 bit are common, but many chips only have 16/24
• [ ] Programs will be expected for months to years at a time
• [ ] Some users want to use the lowest possible power design they can
• [ ] Not all chips have atomics, and may need to use critical sections
• [ ] Users may have other interrupts, some of which may have higher priority than your timer/counter interrupt
• [ ] Users may want to use a global timer inside of other interrupts

[Ben-PH]

meeting reference: https://github.com/rust-embedded/wg/discussions/758#discussioncomment-9511537

[jamesmunns]

CC @rust-embedded/hal team for review - I'm not sure if this is exactly how we decide on HAL traits at the moment (or if this should be in the /embedded-hal repo), but this seems a reasonable place to start the discussion, and certainly is a contentious enough topic historically to potentially shake out in RFC form.

[David-OConnor]

What I think would be useful: Something similar to std::time::Instant's functionality. How I currently handle it, using a HAL hardware timer method:

let timestamp = tick_timer.get_timestamp_ms();
if timestamp - self.last_channel_change > HOP_INTERVAL {
    self.last_channel_change = timestamp;
    // Do things.
}

In ISR, to handle wrapping:

#[interrupt]
/// Increments the tick overflow.
fn TIM3() {
    timer::clear_update_interrupt(3);
    TICK_OVERFLOW_COUNT.fetch_add(1, Ordering::Relaxed);
}

[Ben-PH]

What I think would be useful: Something similar to std::time::Instant's functionality. How I currently handle it, using a HAL hardware timer method:

let timestamp = tick_timer.get_timestamp_ms();
if timestamp - self.last_channel_change > HOP_INTERVAL {
    self.last_channel_change = timestamp;
    // Do things.
}

In ISR, to handle wrapping:

#[interrupt]
/// Increments the tick overflow.
fn TIM3() {
    with(|cs| {
        access_global!(TICK_TIMER, timer, cs);
        timer.clear_interrupt(TimerInterrupt::Update);
  });
    TICK_OVERFLOW_COUNT.fetch_add(1, Ordering::Relaxed);
}

I agree that the question of overflow is... conspicuous. I've made a mental note. Don't know where I'll go with it, but my initial gut is to have a way of registering interupt handlers as part of the interface. somithngi like

fn register_overflow_interupt(&mut self, ???)

to lean into your example, you would pass in TIM3, or an equivilent closure, and the implementation detail when it comes to doing the read would use some sort of code that is in the first half.

That's going to be a headache and a half, but would definately need to be addressed at some point.

[David-OConnor]

Yea - I guess the issue is here, overflow is likely going to happen depending on the underlying time keeper. It will have to be managed. In the example I posted, this is handled using an incrementing atomic wrap counter that both the firmware (to update), and time-getting-code (to use).

The API to get and use the timestamps (Instants etc) shouldn't expose wraps though.

[David-OConnor]

Inspired by this thread, I've updated the usage code (in my code bases) to this; uses an Instant that's similar to std::Instant, and implements addition/subtraction with core::Duration:

let timestamp = tick_timer.elapsed(); // `Instant` type.
if (timestamp - self.last_channel_change).as_millis() > HOP_INTERVAL {
    self.last_channel_change = timestamp;
    // Do things
}

[Ben-PH]

The base of my thoughts is that when you think "counter", especially in an embedded context, the notion of it wrapping is well within scope. Just not sure about what shape it should take at the interface level.

[Ben-PH]

@David-OConnor I've updated the document with an initial attempt to address this. Would be interested in your thoughts:

https://github.com/rust-embedded/wg/pull/762/files#diff-66a0e54eb567d4eac263c2c1fca7467e332d8c8346d98da53ae554159ea97f67R175-R194

[David-OConnor]

I'm not great with trait design, but I think that should work? Overall: The timer struct in question should be able to A: Store a value that counts wraps. B: Be able to increment (or reset?) that counter.

Of note, an alternative is to, instead of specifying it in the trait, is to use a public atomic defined in the struct's module. The struct uses that to calculate elapsed time, and it is updated without access to the struct in a timer interrupt etc. That way ownership, critical sections etc isn't required to handle the wrap.

[Ben-PH]

@David-OConnor I'm on a bit of a mission to fork-and-demo. Basically, I have this repo going: https://github.com/Ben-PH/counter-proposal/ and want to see how it fits together with as many hals/drivers/embedded projects as possible. I've already got this put together: https://github.com/Ben-PH/esp-hal-downstream/commit/ef763f441da77bb6fad809428d26bfc007b858d3 and doing that was quite informative. I would be glad to see what it looks like in the context of your work.

[jannic]

Dirbaio pointed out on matrix that https://github.com/rust-embedded/embedded-hal/issues/357 contains important information about past design considerations that are very much relevant for this RFC.