Skip to content

doc/memos: RDM on a unified low-level timer-API#15046

Closed
ngandrass wants to merge 1 commit intoRIOT-OS:masterfrom
ngandrass:pr/doc/memos/rdm-low-level-timer-api
Closed

doc/memos: RDM on a unified low-level timer-API#15046
ngandrass wants to merge 1 commit intoRIOT-OS:masterfrom
ngandrass:pr/doc/memos/rdm-low-level-timer-api

Conversation

@ngandrass
Copy link
Contributor

In order to continue the discussion from the recent RIOT-Summit...

Contribution description

This RDM gathers demands upon a robust unified low-level timer-API for RIOT-OS.
Based on these, requirements are derived and a potential implementation design
is proposed. Results from a survey of timer hardware peripherals are furthermore
included in the appendix section.

The goal with this effort is not to fully replace existing modules but to offer
a possibility to access timer hardware in a uniform and platform-independent
way, whenever desired.

Please note that this RDM is still in an early phase. In order to provide a
foundation to base discussions on, it currently is very verbose. The additional
content shall allow one to quickly get a glimpse of the topic and take part in
the discussion. Some of these sections are currently also part of the RDM in
order to illustrate our thoughts and steps taken so far. The final version shall
of course be a lot shorter and compact.

It should also be mentioned, that at the moment a prototypical implementation
for the STM32 family is being developed to be used for first evaluations and
benchmarks.

Testing procedure

Your feedback is very appreciated.

Please read the RDM and contribute to the discussion if you like! :)

@ngandrass
Copy link
Contributor Author

Suggested tags:

  • Area: RDM
  • Area: doc
  • Area: timers
  • Discussion: RFC
  • State: WIP

Suggested reviewers:

@MichelRottleuthner MichelRottleuthner added Area: doc Area: Documentation Area: RDM Area: Discussions on RIOT Developer Memos Area: timers Area: timer subsystems Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Sep 18, 2020
@MichelRottleuthner MichelRottleuthner added the Process: needs >1 ACK Integration Process: This PR requires more than one ACK label Sep 18, 2020
@maribu
Copy link
Member

maribu commented Sep 19, 2020

I only briefly skimmed the first part of the RDM, but I have some feedback for the stated goal of the RDM.

So far in RIOT, low level APIs where designed to match and reflect the properties of the underlying hardware as good as possible, so that implementations get lean and efficient. E.g.:

  • High frequency timers usually have more than one timer channel, RTTs and RTCs have no channels
    • This is reflected by the periph_timer and periph_rtt/periph_rtc API quite well
  • RTCs internally represent units as year, month, day, hour, minute, second, RTTs and high frequency timers as a single up-counting integer
    • This is again reflected by periph_rtc vs periph_rtt and periph_timer
  • There are often multiple high frequency timers, but at most one RTT and/or RTC
    • Again, this matches the APIs perfects
  • The hardware often has overflow interrupts for high frequency timers and RTTs, but never for RTCs
    • Again, perfect match with the overflow callbacks only supported by periph_timer and periph_rtt

To be honest, I don't really like the idea of a low level unified API. I think this will result in:

  • More runtime overhead
    • E.g. RTCs and RTTs implementations now have to deal with a device and channel parameter, but neither parameter is actually needed
    • Conversion between RTC-like and RTT-like time representation
  • More effort to implement
    • Features not supported by a specific hardware class (e.g. RTC-like time representation, multiple channels, multiple devices for RTT hardware) need still be implemented. (At least a fallback implementation returning e.g. -ENOTSUP needs to be implemented or at least a function pointer to a common implementation needs to be set.)
  • Code duplication
    • Either all RTCs now need to support RTT-like time representation (and maybe vice versa). At least a call to the conversion functions is now needed for every RTC or RTT implementation
    • Argument checking for channel and device parameter in all RTTs and RTCs will be identical
    • Fall-back implementations of features not supported by a specific hardware class will look very much identical between all implementations of the same hardware class

IMO, if you want an unified API to access timer hardware, it should be based on top of the periph APIs. E.g. the ztimer backends for periph_rtt, periph_rtc and periph_timer could be the base for such a unified API and ztimer could become a user of that. As those backends already fulfill just the requirements of a unified API, little more work than splitting them out of ztimer and making them a standalone module would be needed.

The advantage of a unified API on top would be:

  • Immediate support by all hardware in RIOT
  • No duplicated maintenance effort (unified API is not implemented per MCU and does not need to be maintained for each MCU, but just once)
  • No increase in complexity of the MCU specific timer drivers, as the low level API still matches the hardware well

@benpicco
Copy link
Contributor

benpicco commented Sep 20, 2020

I think an overhaul of the timer API is a worthy endeavour. Right now the periph timer API seems like it was tailored to xtimer (or some other high level timer) with the focus on one-shot events and relative sets, up-counting only.
However if accuracy is important, there is no way around using a dedicated hardware timer and fortunately there are usually plenty available.
A neat problem to solve here would be some way to 'request' a hardware timer instead of having to make sure that a single timer is not accidentally configured for two different purposes. If we were to be fancy we could then use that mechanism for high level timers too and only use virtual timers when hardware timers are exhausted. But let's not get too overboard here 😉

The RTT API leaves out if functions not implemented by the hardware should be emulated in software or left out entirely (see discussion in #13630).
It's also not possible to use the second RTT of the nrf5x family with the current API.
On AVR there is no dedicated RTT, but a timer can be configured to be sourced from a 32kHz clock to serve the same purpose.
This causes some code duplication between timer.c and rtt.c.

I would leave out the RTC in this discussion / RDM. RTC can be implemented on top of an RTT if there is no hardware implementation. It behaves different than a simple timer / counter.
AFAIK lpc23xx is the only platform that does not allow to use the RTC counter register directly as an RTT.
Again a generic RTC -> 1Hz RTT conversion layer is trivial here, if there is ever a desire for such a thing.

The Watchdog also serves an entirely different purpose, let's leave this out of scope.

I've also skimmed the RDM and it looks like so far there is only a description of what hardware timers there are, but no proposal yet as to how such a unified API would look.

Keep in mind that this will be a large undertaking.
We also want to keep compatibility with existing applications without degrading their performance.

@maribu
Copy link
Member

maribu commented Sep 20, 2020

A neat problem to solve here would be some way to 'request' a hardware timer instead of having to make sure that a single timer is not accidentally configured for two different purposes. If we were to be fancy we could then use that mechanism for high level timers too and only use virtual timers when hardware timers are exhausted. But let's not get too overboard here wink

A dynamic resource management for timers sounds like a recipe for disaster. The complexity of this is hardly ever needed, but paid by everyone in terms of increases RAM and ROM consumption, severe degeneration of real time capabilities, increased number of bugs and maintenance.

Let's assume we have 3 timers. One is used for for a high level timer API to implement software timers, one is used to generate PWM, and one is used to generate periodic IRQs for a driver with strict accuracy requirements. Let's also assume that only one of the three hardware timers can be used for PWM and only one has the ability to natively generate periodic timeout IRQs. If we now use dynamic run time allocation, we have to reflect the abilities of each timer correctly and extend the request API to request timers with specific capabilities. Let's assume the PWM is currently unused and the a software timer is set, for which the now unused PWM-capabile timer is used. If aftwards PWM is needed, we would have to

a) Wait until the software timer run on the PWM capible hardware timer expires.
- This would result in severe degeneration of real time capabilities
b) Fail
c) Extend the high level timer API to dynamically relocate software timers from hardware and do internal multiplexing instead

Also: Mixing the use of one hardware timer per xtimer / ztimer and multiplexing of a hardware timers seems to be in general a bad idea, even if statically allocating timers (timer channels) to xtimer and other users:

  • Resources and maintenance of both approachs have to be paid as there can always be more xtimers set than hardware timers present
  • Latency becomes unpredictable, resulting in poor quality of service and reduced real timer performance
  • Complexity increases and corner cases are likely a source of bugs

It's also not possible to use the second RTT of the nrf5x family with the current API.

That's a good argument to extend the periph_rtt API to include also a device parameter. And at that point, we could just use the periph_timer API for both.

On AVR there is no dedicated RTT, but a timer can be configured to be sourced from a 32kHz clock to serve the same purpose.
This causes some code duplication between timer.c and rtt.c.

I see no reason that prevents sharing code between both APIs. But if the APIs of periph_timer and periph_rtt will be merged anyway (which in the context of the nrf5x family makes sense to me), that will be quite a bit easier to implement.

@ngandrass
Copy link
Contributor Author

First of all thank you both for your feedback! Allow me to leave my thoughts below.


@maribu Sep 19, 2020, 8:12 AM GMT+2:

  • High frequency timers usually have more than one timer channel, RTTs and RTCs have no channels

RTCs may not have "channels" but "alarms" which basically behave like compare channels. They can be armed to an absolute counter/time value and generate an interrupt once they are reached. On STM32 for example the RTCs feature two alarms, on TI MCUs even three, out of which only one can be used with periph_rtc. Timers that are currently driven via periph_rtt actually do have compare channels (e.g. on SAM3, STM32, EFM32/EFR32/EZR32, ...). IMHO it would be very useful to expose A) the additional RTC alarms and B) the RTT channels.


@maribu Sep 19, 2020, 8:12 AM GMT+2:

  • RTCs internally represent units as year, month, day, hour, minute, second, RTTs and high frequency timers as a single up-counting integer

You are absolutely right here. With the current API-design additional conversion functions would be necessary. However, fully replacing existing APIs is not the goal here. If one requires minimal latency, using periph_rtc directly would still be the way to go. On the other hand RTCs usually are used with long running timeouts in the range of seconds up to hours or even days. Having to convert a relative counter value into a word-clock timestamp should be a reasonable overhead for gaining the benefits of the unified API, if desired by the application developer.


@maribu Sep 19, 2020, 8:12 AM GMT+2:

  • There are often multiple high frequency timers, but at most one RTT and/or RTC

On STM32 I faced the problem that the LP-timer is exposed hard-coded via periph_rtt. This unfortunately leaves the second LP-timer, which is available on some of the STMs, unusable. For me it would be desirable to be able to use all available timers, especially the "very precious" low-power timers.


@maribu Sep 19, 2020, 8:12 AM GMT+2:

The hardware often has overflow interrupts for high frequency timers and RTTs, but never for RTCs

  • Again, perfect match with the overflow callbacks only supported by periph_timer and periph_rtt

Overflow callbacks, as I see it, are not covered by periph_timer. May I have overlooked something here? It might be noteworthy that most of my RIOT experience is with STM devices. Maybe the overflow handling is implemented differently here?

In addition, the callback function typedef void(* timer_cb_t) (void *arg, int channel) indicates a channel but not the actual interrupt cause (or at least it is not uniformly specified by periph_timer). It would be nice to have either the interrupt cause passed as an argument or to be able to attach different ISRs for OVF and CMP_MATCH. See further: 4.x Interrupt handling


@maribu Sep 19, 2020, 8:12 AM GMT+2:

To be honest, I don't really like the idea of a low level unified API. I think this will result in:
[...]
IMO, if you want an unified API to access timer hardware, it should be based on top of the periph APIs.

I'd again would like to outline that the goal is not to fully replace existing APIs but to provide an additional uniform API that offers all the described features (e.g. exposing all timers and channels, allowing usage of advanced timer functions, ...). If an application scenario requires the most direct and basic timer hardware access, existing APIs can still be used directly. Code of these can furthermore be reused inside the aspired unified API to provide all base functions, hereby eliminating the problem of code duplication and also keeping maintainability. Additional code would then only be required for the additional features.

As those backends already fulfill just the requirements of a unified API, little more work than splitting them out of ztimer and making them a standalone module would be needed.

True, but this would leave out all of the aspired enhancements described in the RDM. Current low-level timer-APIs are limited to the most basic set of features that is commonly found across all MCUs supported by RIOT. As addressed in the RDM, many timers actually do offer a lot of additional features that currently either need to be manually implemented by the application developer or are not usable at all. I would very much like to see a platform-independent way of using these as well as the ability to expose the full set of available timer peripherals.


@benpicco Sep 20, 2020, 2:15 AM GMT+2

If we were to be fancy we could then use that mechanism for high level timers too and only use virtual timers when hardware timers are exhausted. But let's not get too overboard here

This actually is on my list of "long-term optimization wishes" ;D


@benpicco Sep 20, 2020, 2:15 AM GMT+2

I would leave out the RTC in this discussion / RDM. RTC can be implemented on top of an RTT if there is no hardware implementation. It behaves different than a simple timer / counter.
[...]
The Watchdog also serves an entirely different purpose, let's leave this out of scope.

The fact that an RTC can be implemented on top of an RTT is IMHO an argument to actually keep the RTC within the scope of the RDM. It shows that the functionality is that similar, that an RTC can even run on top of an RTT. It therefore should be included in a unified API.

Regarding the watchdogs: Yes, as stated later in the RDM I also consider them as out of scope for the API.


@benpicco Sep 20, 2020, 2:15 AM GMT+2

I've also skimmed the RDM and it looks like so far there is only a description of what hardware timers there are, but no proposal yet as to how such a unified API would look.

See # 2. Demands on a Low-level Timer Subsystem and # 4. Low-level Timer API Design :)


@benpicco Sep 20, 2020, 2:15 AM GMT+2

Keep in mind that this will be a large undertaking.
We also want to keep compatibility with existing applications without degrading their performance.

Sure thing! This is nothing that I want to rush. The purpose of this PR is to get the initial discussion going, not to get it merged and implemented next week.

@benpicco
Copy link
Contributor

Many timers also provide a Capture functionality that is unfortunately not exposed by any RIOT API yet.
There a GPIO is tied to a timer channel and if the GPIO level changes the timestamp of that event is stored in the Capture channel.

Should this also be within the scope of this API?

@maribu
Copy link
Member

maribu commented Sep 21, 2020

I see no reason not to extend periph_timer to include a callback on overflows or expose the capture feature. I'm personally very interested in the latter and had just this on my to-do list. (This could be used to get a time stamp when an IRQ happened on a network device, which in turn could be used to implement RX and TX timestamps, which in turn could be used for PTP time synchronization.)

I do understand the desire to unify periph_timer and periph_rtt. I believe @MichelRottleuthner is also strongly in favor of doing so and no one spoke against it. (Except for me, but count me convinced now.) Thus, I think such an undertaking would have enough support to get upstream rather swiftly.

But I really see no reason why uniform access to high speed timers, RTTs and RTCs should not be implemented on top. This could be done e.g. by using a translation layer between the merged periph_timer and periph_rtt API and the periph_rtc API. I really don't see any benefit for every driver to manage unit conversions, when this can be done just as well in a single place on top.

@MichelRottleuthner
Copy link
Contributor

Thank you all for spending time on this topic!

(..) Capture functionality (..) Should this also be within the scope of this API?

If you ask me: absolutely within scope! Did someone say PWM? :P
Same for the 'neat request a timer' thing you mentioned above. This actually popped up a couple of times in the past already.
Either all people thinking in that direction are morons, sorry for you and me, or there are valid use cases for that.
Of course no one would ever want to use that in the 'weird-dynamic' way @maribu described above.
But thinking about whether something like that is possible, reasonable, and under which conditions is valuable.
An optional allocation mechanism for timer resources, why not ?
...but yeah, I agree we can only go one step at a time...

But I really see no reason why uniform access to high speed timers, RTTs and RTCs should not be implemented on top. (..)
I really don't see any benefit for every driver to manage unit conversions, when this can be done just as well in a single place on top.

I think we can all agree that the question on whether a unified API makes sense can be mostly separated from the details on how exactly it is implemented in the end.
Why I think we can agree on a unified timer API being a reasonable goal:
In the end it is just an explicit more elegant way of doing what is already done behind curtains.

  • RTT implementation being internally mapped to the RTC and the other way around
  • utility module converting between RTT and RTC
  • Custom periph_timer implementation internally mapping to low power timers
  • ztimer mapping any backend to the same internal interface
    • In case of RTC and RTT this pulls in the specific APIs and implicitly blocks them for the rest of the application.
    • This also illustrates there is quite some common functionality across all timers that other layers may want to use uniformly. In this case it is ztimer. It could also be something else that needs access to more than one hardware timer instance.

"every driver to manage unit conversions" of course sound like a bad idea. So why would you want to do that? Of course we want avoid that wherever possible. Common utility functions? Other solutions?

A lot of this topic boils down to having a solid (wide and detailed) understanding of the hardware itself - and more importantly some form of collective agreement on this bigger picture.
How can an abstraction be "designed to match and reflect the properties of the underlying hardware as good as possible, so that implementations get lean and efficient" if you don't know what precisely it is that you are abstracting there?

I see absolutely no reason to rush any of this. Proper discussion and thinking it through will take a lot of time but I think it will be well spent as long as the generated knowledge is well documented and preserved for the community.
The community as a whole can learn from that and build upon that knowledge - no matter if heading for success or failure.
Worst case: in the end there will be a document that describes in detail what of this approach doesn't work and why.
Best case: up to your imagination ;)

@maribu
Copy link
Member

maribu commented Sep 22, 2020

I think we can all agree that the question on whether a unified API makes sense can be mostly separated from the details on how exactly it is implemented in the end.

Not having to maintain and implement two functionally equivalent low level APIs for the very same features for every MCU family is IMO a requirement, not an implementation detail. And I find it hard to find any argument against this.

The discussion about the general architecture was started by this RDM, which clearly wants the API implemented on per MCU family side by side to the existing timer APIs. If the RDM is updated to leave the discussion about the general architecture for later, I would be fine.

Side note: I have nothing against discussion on whether a new low level timer API should replace all periph_timer, periph_rtt and periph_rtc implementations. A compatibility layer providing the old APIs on top of the new API for legacy code would also avoid having to maintain two APIs. (I'm personally still in favor of improving the existing APIs and build a unified API on top, but I'm certainly open for arguments and definitely won't block this.)

But lets please leave watch dog timers and PWM out of this. E.g. I see no reason why it shouldn't be allowed to implement PWM abusing I2S and DMA when a specific MCU has no PWM capable timer. To me, PWM has just as much to do with timers as QDEC has: They just happen to be mostly implemented reusing timer hardware.


lt;dr: Let's limit this RDM to the discussion of requirements for level timer APIs and only consider "real" timer features (not watchdogs, not PWM, not QDEC).

@benpicco
Copy link
Contributor

benpicco commented Sep 22, 2020

Same for the 'neat request a timer' thing you mentioned above. This actually popped up a couple of times in the past already.

@maribu is right though.
e.g. on sam0, timers can only have 8 different prescalers to derive the timer frequency from the input clock.
The input clock can also be prescaled from it's clock source, allowing for a wider range of resulting frequencies.
However the input clock can be shared with other peripherals, so we can't give the timer driver full control over it to figure out the best divider combination for a requested input frequency.

So we can have timers that can reach different frequencies with varying accuracy depending on the input clock.

However, such timer request mechanism is orthogonal to a timer API, we can just leave that out of scope.

@MichelRottleuthner
Copy link
Contributor

Not having to maintain and implement two functionally equivalent low level APIs for the very same features for every MCU family is IMO a requirement, not an implementation detail.

Functionally equivalent parts don't need to be reimplemented. And no one want to just duplicate code here.

If the RDM is updated to leave the discussion about the general architecture for later, I would be fine.

I don't say the RDM should be split for that. The arguments for and against abstract concepts and implementation details should be focused.

I'm personally still in favor of improving the existing APIs and build a unified API on top, but I'm certainly open for arguments and definitely won't block this.

Perfectly fine. Just keep in mind that achieving the envisioned feature-set of the proposed solution will require extensions to all of the existing APIs. Many of them will share a lot of similarity, effectively leading to what you want to avoid: "re-implementing the wheel" and code duplication.
Properly testing all the timers will be another aspect where unification will eventually come in handy.
After collecting all the relevant metrics on how the proposed approach performs and how it can serve our needs we can look at the data and discuss if it is worth keeping different level entry points via separate APIs.

But lets please leave watch dog timers and PWM out of this.

Regarding watch dogs: already stated in the RDM and the PR discussion.
PWM: shouldn't be decided with a "quick shot" IMO. But yes not tho core problem and a rather high hanging fruit compared to a lot of other stuff with much higher priority.

just happen to be mostly implemented reusing timer hardware
(..) and only consider "real" timer features

I guess I have to adapt my definition of "real" then...

However the input clock can be shared with other peripherals, so we can't give the timer driver full control over it to figure out the > best divider combination for a requested input frequency.
So we can have timers that can reach different frequencies with varying accuracy depending on the input clock.

Well known. A topic that overlaps with clock configuration and will be tackled separately. For the the timer API these aspects are only important regarding documentation and scope.

However, such timer request mechanism is orthogonal to a timer API, we can just leave that out of scope.

Agree.

@maribu
Copy link
Member

maribu commented Sep 22, 2020

Not having to maintain and implement two functionally equivalent low level APIs for the very same features for every MCU family is IMO a requirement, not an implementation detail.

Functionally equivalent parts don't need to be reimplemented. And no one want to just duplicate code here.

That's all I asked for :-)

@stale
Copy link

stale bot commented Jun 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jun 3, 2021
@tcschmidt
Copy link
Member

Ping @ngandrass

@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Jun 3, 2021
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022
@stale stale bot closed this Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: doc Area: Documentation Area: RDM Area: Discussions on RIOT Developer Memos Area: timers Area: timer subsystems Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: needs >1 ACK Integration Process: This PR requires more than one ACK State: stale State: The issue / PR has no activity for >185 days State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants