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
• 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: #758 (comment)

[jamesmunns]

A few notes, as somebody not on the HAL team, but present for many of the previous discussions.

[jamesmunns]

Would this interface work well with shared access? e.g. &ClkSrc?

[Ben-PH]

I don't see why that should be difficult, and if it was, why it should not be overcome.

One thing to consider, is the question of how this should differ from, say, an InputPin implementor. conceptually, it's no different. instead of reading an inherently boolean state, it's a count, with the addition of a specification of what's being counted.

[jamesmunns]

This typically becomes challenging if you need to have more complex inner state - e.g. expanding a 16-bit timer to larger ranges. Something to keep in mind if you need to change other parts of the design.

[jamesmunns]

Noting that this probably need to be fallible, for example if we are more than 66 seconds into the runtime.

[Ben-PH]

agreed. This specific code is useless outside of the scope of illustrating a point. Key ideas is to communicate a potential idiom.

[jamesmunns]

Does this address platforms where only down-counting timers exist? Are implementors expected to handle this (e.g. u32::MAX - raw_value?)

[Ben-PH]

At this point no. A good design would be mindful of behavior where countdown is a posibility, but for now, I'm thinking in torms of strictly incrementing counts.

[jamesmunns]

Gotcha - something to keep in mind. I have go figure out what platform it is, but IIRC a fairly typical platform only has down-counting timers available.

[Ben-PH]

this is the assosciated type I'm going with at this point (might drop the Copy):

    /// Typically u32 or u64 (or held within a type-wrapper) that corresponds with the
    /// data type held within the peripheral register being read.
    type RawData: Copy;

I've been ruminating on this question a bit, and I can't find any technical, development, portability, etc. reason not to have this on the forefront. Right now, the only thing stopping me, is finding a chance to do a bit of a case-study of making a hal/driver/etc portable through this trait, and needs this behavior yo describe.

I anticipate it being (relatively) trivial. My guess is a hal impl would implement try_read_raw, and do the mapping you describe. They would set the raw-data type to be u32, do the u32::max - $REGISTER_READ (or similar), then provide their own mappings to a data type that encapsulates the thing they are measuring.

[jamesmunns]

If u32 is used, is there an opinion of what to do in the case of wraparounds? For example, 1MHz based timers are somewhat common and desired for timing, but this will wrap around a u32 in approximately 71 minutes. Even a 32.768kHz time will wrap a u32 in 1.5 days.

A u64 can probably practically be considered infinite, even at 1GHz, it would take ~584 years to wrap a u64.

[Ben-PH]

That would be up to the implementor.

[jamesmunns]

"that would be up to the implementor" - but this affects downstream users (e.g. drivers consuming the trait). Can drivers rely that the number always goes up? If so, how is this behavior ensured if the implementor provides a 1MHz 32 bit timer that works for the first 71 minutes, but not afterwards?

[jamesmunns]

Nailing down this kind of three-way contract between the HAL implementor, the trait interface, and the trait-using driver is pretty much the whole challenge of stabilizing these APIs, there's only so much that can be left to the implementor, while still providing a useful contract to the consuming driver.

[Ben-PH]

I've added a draft of a concept where you can extend a time-counter:

/// There are many scenarios where a time-based tick-counter needs to handle the case
/// where the counter overflows. This trait is intended to provide a HAL with the means
/// of providing a `TimeCount` implementor with specialised interupt-based overflow
/// tracking. 
pub trait TimeWrap: TimeCount {
    /// A HAL implementation might register the provided method to handle an overflow
    /// interupt and increment a field that tracks overflow.
    ///
    /// META: I honestly don't know the best way to do this. This might prove to be
    ///       a key technical challenge
    fn register_wrap_interupt(self, ???);
}

Is it the good way? dunno. Keen to hear thoughts.

[jamesmunns]

Does this RFC propose a mechanism for multiplexing multiple timers, e.g. if new_timer is called multiple times? Is this Timer interface specified anywhere?

[Ben-PH]

I'll have to think about this, but with respect to constructors, I anticipate not including it, much the same as with all the other traits in embedded-hal

[jamesmunns]

I think that's reasonable!

[Ben-PH]

If you mean "new_timer returns a new instance, but sometimes each instance just uses the same timer under the hood. What approach to allowing this is being proposed?", I see this as addressing the challenge of "shared state, not necisarily immutable", and using a multitude of multiplexed objects as one particular strategy.

I'm of the mind that the primary domain of portability sits within the the scope of how a thing is used. There could be a case for a "soft-standard" of construction, but the driving mandate should always be "portability in its use".

Some thoughts I've been reminating on:

• Object exclusivity contradicts the premise of portability of count-readers. Time-based, or otherwise.
• Shared mutable state is to be avoided, especially at the interface level.
• Not being able to multiplex, or use other means to share a counter that's potentially mutable, is a deal-breaker.
• Interior mutability patterns/idioms/lang-features/etc. at impl level should be able to resolve this contradiction nicely. Their use must not be limited in any way.
• In particular, w.r.t. embedded, interior mutability must be available in the context of juggling interrupts, soft/hard-real-time use-cases, etc.

My gut is to use patterns of shared immutable state at the interface level, and ensure no restrictions are imposed to allow for sharing something that is imutable under the hood

To whit:

• &self in the interface
• Investigate the various multiplexing approaches used, and ensure that e-hal Just Works to make their solutions usable in a portable fashion.
• Validate this in the context of embedded: interupts, real-time, etc.

If a particular implementation chooses to use multiple, unshared, mutable objects that multiplex a single thing under the hood, I don't see how that would be incompatible with the pattern I just proposed at first glance, but I expect to be proven wrong on that.

[jamesmunns]

As mentioned, it is generally required to first get a working system, usually with at least 2-3 platform implementations, EXTERNALLY first, so that they can be evaluated in working operation, prior to merging anything or creating new repos in the rust-embedded space.

[Ben-PH]

Understood, but I'm concerned about the chicken-egg paradox.

[jamesmunns]

Sure, however the HAL team is just as concerned about having/owning a crate that it isn't able to consider the design of.

[Ben-PH]

I see that there is much context I do not account for in my original intent. I'll educate myself on the context/standards/expectations and update my declared thoughts on moving forward.

[jamesmunns]

If the underlying counter ticks are truly decoupled from the concept of time, it would be good to discuss how this would work in practice. Many platforms don't have hardware floating point support, and still many platforms (Cortex-M0, M0+, likely others) don't necessarily have hardware division support.

Will it be reasonable for these platforms to perform the tick->time conversions?

[Ben-PH]

That would be up to each platform implementation. Fugit does everything using integers and in cost time with minor exceptions, and I wouldnt be surprised if platforms gravitated towards fugit primitives when counting for time.

[jamesmunns]

Not saying this is a deal breaker - but something to actively address and engage with. If the base timer is 1MHz, it'll require integer division to render milliseconds or seconds.

It would be good to discuss, even if the answer is "fugit does it like this, it is this cheap, reasonably priced for the features provided".

[Ben-PH]

One way of interpreting this comment, is that it's a proposal to include a canonical expression of time when writing portable embedded. Either as part of this RFC, or in a seperate, but closely related one.

Here are my thoughts so far, from most, to least portable:

• fugit: u32 or u64. extreme emphasis on zero runtime costs through the use of consts
• embassy time: u64 only
• core library is a u64 and u32, and doesn't use the concept of "ticks"

For me, the easiest answer to what is most portable is "whatever type you choose", but that's not much use, I think this is a void that needs filling.

My take is that at least the following properties are needed in order to be considered maximumly embedded-portable:

• Is based on raw-data, such as tick-count (fugit, embassy-time)
• Raw data is of any type. If it's not an integer type, it should be a thin-wrapper around an integer type (Foo(T), with T being any integer)
• A compile-time encapsulation of scale. For time, this would be the tick-frequency. (fugit, embassy_time)
• Zero-cost-abstraction aliasing between common representations, such as millis, micros, days, etc. (fugit, embassy-time)
• A set of type-aliases that explicitly represent different time measures without cost, such as MilliSeconds<u32> (fugit)
• implementation of common conversions meeting zero-cost-abstraction, such as Into<embassy_time::Duration/Instant>, Into<core::time::Duration>, should be trivially easy. A code-base that uses portable e-hal time, then calls into embedded-time, the part of the code that does the type-conversion should be optimised away in every-respect before the executable binary is generated.

I feel the closest is fugit, but for me to feel fugit as ready to integrate into e-hal, I percieve these as some of the blocking issues.

• u32 and u64 only. that covers the vast majority of platforms, perhaps, but u8, 16, 128 exist as well, and for domains other than time, such as pulse-counters, you might want signed values. Multiple values are also a possible scenario (a tick-counter and a rollover counter, for example).
• It uses const-generics for encapsulating scale (e.g. a 80hz u32 tick-count would be Instant<u32, 1u32, 80u32>), which means integrating with other interfaces is hit-or-miss (they might only be able to use usizes, or if they use u32, and you want to bridge the two, you're stuck with u32 types). It makes no sense to me that code must choose a machine-representation of a number to describe the frequency of an oscilator.
• It's waiting on some unstable const-generic features to add useful features (e.g. to do changing between bases).
• It's limited to time-only: If you want to count, for example, milliradians with a magnetic AB rotary encoder, that could work perfectly fine, but the naming would be Bizare.

I would be 100% behind setting up some sort of e-hal encapsulation of measuring time, and other things that counters... count, but there's some work involved to get there, imho.

[jamesmunns]

As a note: embassy-time is fully usable in non-async contexts. There is no requirement to use async at all. There are certainly other drawbacks you could mention (requirements to use u64, separate externally defined driver methods, mandated timer wheel/queue implementation, etc.), but these are fully separate from "async or not".

[Ben-PH]

I should add that embassy time is not intended to be a universally applicable encapsulation of a counter, or something along those lines.

[jamesmunns]

It would be good to fully demonstrate what the typenum-based interface (or any alternatives) would look like for users. See my top level notes wrt providing examples for discussion.

[jamesmunns]

I appreciate you writing this up! In particular, I would very much like to see the following items, which I don't see in the current draft:

• discussion of potential wraparound conditions, particularly if u64 is not a mandatory representation of the counter value
• discussion of how you expect platforms with limited range timers (e.g. 16-bit only timers) to participate for u32 and u64 based counter values
• If necessary (particularly for increasing smaller timers to larger ranges), any external resources, such as interrupts, that may be necessary for implementation
• How you expect sharing of these clock/counter resources to work, for example if multiple drivers need to use them to provide timing capabilities
• Any additional API surface, such as how "wait for timer to complete" would look

I'd also really really like to see:

• The concrete traits you are proposing, for example in a a published crate (on GitHub or crates-io)
• At least one example of implementing these traits, for your hardware platform or HAL of choice, likely in a fork of an existing HAL
• At least one example of a downstream driver crate, that uses this trait
• At least one example of an application, that links the HAL-provided trait impl and the trait-consuming driver, e.g. an "end to end example"

These examples are vitally necessary for evaluation - both for you to determine if the proposed interface(s) are reasonable in practice, as well as for others to comprehend the full extent of what you are proposing.

I'm not on the HAL team, but I imagine all the items I have listed here (and in comments) will be strong points of discussion, so for your RFC to be accepted it should likely address or dismiss these concerns.

[Ben-PH]

Your first group of points illustrate the need to ensure that everything that is implementation detail be left to the implementor. If a hal author of a specific platform or usecase is in any way hampered in their ability for their implementation to be encapsulated appropriately, then that is a failure of the trait definitions. For example, platforms with 16 bit counters could track wrappings themselves, or they could just say "out counter wraps" or whatever they choose to do.

The proposal in this RFC is to standardise an interface, with the only restriction on implementation detail being at the type constraint level.

[jamesmunns]

Yes, however, it is necessary to at least demonstrate that your proposed interface is practically usable before agreeing on that - and the best way we have today is by providing at least one (ideally a few diverse) implementations.

We've had many designs in embedded-hal that SEEMED like good ideas, or were good in isolation, but did not interact well beyond basic "hello world" interactions. This is particularly true in SPI and I2C, where it became clear between 0.2 and 1.0 that it was necessary to address sharing of a bus, as many practical systems had more than one device on the bus.

[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: Ben-PH/esp-hal-downstream@ef763f4 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 rust-embedded/embedded-hal#357 contains important information about past design considerations that are very much relevant for this RFC.