Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

periph/countdown: introduce Countdown timer interface #17159

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Nov 7, 2021

Contribution description

When working on speeding up the DOSE driver, I noticed that we don't have a fast interface to set countdowns.

The periph_timer API is targeted towards timekeeping with a continuously running timer. This means that for setting an alarm, we first have to read the current time, add the alarm period, then set the alarm.

When reading the current time involves syncing clock domains, this becomes a costly process that does not allow us to keep up with line data rates of 1 MBit/s (10 µs/byte on UART).

Then there are timers (SysTick) that don't have a notion of 'current time', but will count down from a set time until they reach zero.

This prompted me to introduce a new API explicitly for countdown devices that is targeted towards speedy operation.

It is split into a low-level device API and a high-level user API which allows the device drivers to remain simple and to share common code across implementations.

As an example, a period extension has been implemented periph_countdown_long that allows to set alarms further in the future than the hardware timer would allow.
This is of course more expensive than a normal countdown_set() so it should not be used in time critical sections.
countdown_start() however remains fast.

So far implementations are provided for Cortex-M SysTick and Atmel SAM Tc and Tcc timers (tested on samr21-xpro and samr54-xpro).

Testing procedure

A test application is provided in tests/periph_countdown.
It will execute API tests on all available Countdown timers, then benchmark the API operations.

samr21-xpro

2021-11-07 20:17:28,858 # main(): This is RIOT! (Version: 2022.01-devel-423-g4ac7f-systick-countdown)
2021-11-07 20:17:42,028 # .......
2021-11-07 20:17:42,029 # OK (7 tests)
2021-11-07 20:17:42,030 # 
2021-11-07 20:17:42,031 # --- Benchmark ---
2021-11-07 20:17:42,037 # countdown[0].set()	2717 ns/call
2021-11-07 20:17:42,040 # countdown[0].start()	946 ns/call
2021-11-07 20:17:42,044 # countdown[0].cancel()	907 ns/call
2021-11-07 20:17:42,053 # countdown[1].set()	6009 ns/call
2021-11-07 20:17:42,058 # countdown[1].start()	1321 ns/call
2021-11-07 20:17:42,064 # countdown[1].cancel()	3010 ns/call
2021-11-07 20:17:42,073 # countdown[2].set()	6010 ns/call
2021-11-07 20:17:42,077 # countdown[2].start()	1345 ns/call
2021-11-07 20:17:42,083 # countdown[2].cancel()	3010 ns/call

same54-xpro

2021-11-07 20:16:42,402 # main(): This is RIOT! (Version: 2022.01-devel-423-g4ac7f-systick-countdown)
2021-11-07 20:16:55,540 # .......
2021-11-07 20:16:55,541 # OK (7 tests)
2021-11-07 20:16:55,541 # 
2021-11-07 20:16:55,542 # --- Benchmark ---
2021-11-07 20:16:55,546 # countdown[0].set()	769 ns/call
2021-11-07 20:16:55,549 # countdown[0].start()	227 ns/call
2021-11-07 20:16:55,552 # countdown[0].cancel()	202 ns/call
2021-11-07 20:16:55,556 # countdown[1].set()	1252 ns/call
2021-11-07 20:16:55,559 # countdown[1].start()	268 ns/call
2021-11-07 20:16:55,563 # countdown[1].cancel()	502 ns/call
2021-11-07 20:16:55,567 # countdown[2].set()	1294 ns/call
2021-11-07 20:16:55,570 # countdown[2].start()	310 ns/call
2021-11-07 20:16:55,573 # countdown[2].cancel()	419 ns/call

Issues/PRs references

useful for #17097

@github-actions github-actions bot added Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: tests Area: tests and testing framework Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Nov 7, 2021
@benpicco benpicco added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Area: timers Area: timer subsystems labels Nov 7, 2021
@github-actions github-actions bot removed the Area: timers Area: timer subsystems label Nov 8, 2021
@benpicco benpicco requested a review from jue89 February 3, 2022 10:21
@benpicco benpicco force-pushed the systick-countdown branch 2 times, most recently from febe804 to 5ab8297 Compare February 25, 2022 15:00
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Feb 25, 2022
@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 3, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 3, 2022
@riot-ci
Copy link

riot-ci commented Nov 4, 2022

Murdock results

FAILED

ddb027f Update cpu/sam0_common/periph/countdown.c

Success Failures Total Runtime
5255 0 6826 07m:56s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see inline

cpu/sam0_common/periph/countdown.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/countdown.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/countdown.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/countdown.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/countdown.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/countdown.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/countdown.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/countdown.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/countdown.c Outdated Show resolved Hide resolved
Comment on lines +94 to +95
uint32_t ticks_per_unit; /**< counter ticks per base unit */
uint8_t base_unit; /**< base unit (ns, µs, …) of the counter */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this increase complexity? Why not just uint32_t freq_hz?

What if ticks_per_unit ends up being 1.5? This would add up to large errors quickly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was that you typically don't (or shouldn't) need to care about the timer frequency.
So you only tell countdown_init() about the time range you want to have for the countdown and then it figures out the best frequency on it's own (where ticks_per_unit would be a whole number).

I think it was then easier to calculate the actual timeouts based on that (since you can just multiply with ticks_per_unit).

It's been a while since I while since I wrote this, might have to take a look at the API again to see if it still makes sense.

@maribu
Copy link
Member

maribu commented Jan 5, 2023

Ping? :)

@benpicco benpicco added the State: waiting for author State: Action by the author of the PR is required label Feb 2, 2023
benpicco and others added 15 commits February 2, 2023 13:51
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
Co-authored-by: Marian Buschsieweke <maribu@users.noreply.github.com>
@benpicco benpicco removed the State: waiting for author State: Action by the author of the PR is required label Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants