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

sys: add new timer subsystem #2926

Merged
merged 8 commits into from
Aug 25, 2015
Merged

sys: add new timer subsystem #2926

merged 8 commits into from
Aug 25, 2015

Conversation

kaspar030
Copy link
Contributor

This is my work-in-progress new timer implementation.

Please take a look...

The working title is "wtimer", but as that is hard to pronounce in english, I'm very open to suggestions.

It's now called xtimer, leaving room for two more tries! ;)

In order to actually try the code, I've adapted vtimer_msg to this API, the test is in test/wtimer_msg.

(PR depends on #2870 so it can be tested on native).

To do:

  • support for base timer with a speed != 1MHz
  • use slow low power timer or RTC if available (let's get this in first)
  • removal of timers
  • some of the convenience functions are only available in 32bit versions
  • some functions are missing

Optional:

  • per-platform tuning values
  • setting timers from whithin callbacks needs special care (half fixed)

@kaspar030 kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: new feature The issue requests / The PR implemements a new feature for RIOT Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Impact: major The PR changes a significant part of the code base. It should be reviewed carefully State: waiting for other PR State: The PR requires another PR to be merged first TimerTaskForce labels May 6, 2015
@haukepetersen
Copy link
Contributor

Hm, didn't we talk about that relying on a 1MHz timer is not a good idea?

@OlegHahm
Copy link
Member

OlegHahm commented May 6, 2015

Just a (maybe not so tiny) issue: the current timer system revealed a lot of bugs particularly on the 16-bit (aka MSP430) platforms. So, I think it is required to test this on these boards. Caveat: so far, we don't have periph/timer there.

@kaspar030
Copy link
Contributor Author

Hm, didn't we talk about that relying on a 1MHz timer is not a good idea?

Uh, I thought that we agreed that we could get a timer close (small factor) to that and then integrate this factor.

@kaspar030
Copy link
Contributor Author

Just a (maybe not so tiny) issue: the current timer system revealed a lot of bugs particularly on the 16-bit (aka MSP430) platforms. So, I think it is required to test this on these boards. Caveat: so far, we don't have periph/timer there.

I did some tests with both 24bit and 16bit timers, so they should work with this, but yeah, please break it. ;)

@kaspar030
Copy link
Contributor Author

A question came up when developing:

Currently, removal of a timer would be O(n) with n being the number of active timers. It is possible to get this to O(1) by using doubly linked lists, but that would increase the timer struct by 4 byte (from 20 to 24), and increase the codesize because of the handling of that pointer.

Opinions?

@OlegHahm
Copy link
Member

OlegHahm commented May 7, 2015

I think O(n) is perfectly fine for timer removal as long as this is not called internally for whatever reasons. Calling Xtimer_remove() from outside the timer module itself has been definitely a rather rare case so far.

Edit: For me this is a obvious case for memory efficiency over speed performance as long as it is not required to call this function from a critical context (e.g. ISR).

@miri64
Copy link
Member

miri64 commented May 7, 2015

Actually I use it in the Neighbor Discovery (though I don't know if my usage is correct): Neighbor Discovery messages are not really periodical but have random resend times, as soon as a new message is send out. So what I currently do is e.g.

vtimer_remove(&iface->rtr_adv_timer);
vtimer_set_msg(&iface->rtr_adv_timer, interval, thread_getpid(), NG_NDP_SEND_RTR_ADV, pkt);

@OlegHahm
Copy link
Member

OlegHahm commented May 7, 2015

The current usage of vtimer_remove() is mostly necessary because of a bug in vtimer.

@kaspar030
Copy link
Contributor Author

@OlegHahm What bug is that?

Thinking about it, O(n) in this case will be a couple of instructions per timer. IMHO that would be acceptable, considering that everywhere else the additional instructions for filling the second pointer in the lists are saved.

Currently, at each short timer overflow (e.g., every 71,6 minutes for 1mhz 32bit timers, more often for 16bitters), the ISR does a copy of long timers into the short timer list. This is also O(n) with n being the number of timers that have to be copied. I don't know how to make this O(1) without using a lot more lists...

@OlegHahm
Copy link
Member

OlegHahm commented May 7, 2015

@kaspar030, to be honest, I don't remember. That was more than a year ago. I think we realized a race condition if the timer fires while it's getting deleting or something like this. So to be on the safe side we prepended every timer_set with a timer_delete. However, it might be that the bug has been fixed in the mean time.

@kaspar030
Copy link
Contributor Author

Ah, so people set timers that are already set, with a different target time? I need to check for that.

@miri64
Copy link
Member

miri64 commented May 12, 2015

The working title is "wtimer", but as that is hard to pronounce in english, I'm very open to suggestions.

KISS: vtimer2.

@OlegHahm
Copy link
Member

What about ttimer (pronounced like tea timer)? 🍵

@OlegHahm
Copy link
Member

Ah, so people set timers that are already set, with a different target time? I need to check for that.

Yes, I think I remember again what the problem was: if you set the same timer twice, it's added to the queue again and creates a circle.

@lightblu
Copy link

Hm, didn't we talk about that relying on a 1MHz timer is not a good idea?

Uh, I thought that we agreed that we could get a timer close (small factor) to that and then integrate this factor.

I'm wondering too if this is not a too limiting decision. Can really every platform so far provide a 1 MHz timer? Because it looks like Freescale's Freedom boards are set up so that the low power timer could only use the 32 kHz clock as source in the low power modes (but I have to recheck that in detail).

@jnohlgard
Copy link
Member

Because it looks like Freescale's Freedom boards are set up so that the low power timer could only use the 32 kHz clock as source in the low power modes (but I have to recheck that in detail).

The low-power timer in Kinetis processors can only run at 1 kHz or 32.768 kHz, at least on K60. All other timers stop when entering the power saving modes, so they are not suitable for using as a wakeup source.

@kaspar030
Copy link
Contributor Author

the plan is to use a two-timer system, one with higher resolution and a fall-back to the lower resulution low power timer.

@OlegHahm OlegHahm modified the milestone: Release 2015.06 May 28, 2015
@OlegHahm OlegHahm removed their assignment May 29, 2015
@jnohlgard
Copy link
Member

I don't understand the Travis failures.

failed: timer_periodic_wakeup

Errors: 0 Warnings: 0

The command "./dist/tools/travis-scripts/build_and_test.sh" exited with 1.

but timer_periodic_wakeup was never built for any platform during the compile test.

Building application: timer_periodic_wakeup (6/6) 

Building all applications in: tests

@kaspar030
Copy link
Contributor Author

@gebart I've moved up timer_periodic_wakeup one but forgot to change the Makefile's $RIOTBASE setting.

@kaspar030
Copy link
Contributor Author

Maybe I should remove the example until all platforms support periph/timer, or the test can depend on FEATURE_PERIPH_TIMER... What do you think?

@jnohlgard
Copy link
Member

I think the example itself is useful. I think using FEATURES_PERIPH_TIMER would be the proper way to filter out the unsupported platforms.


kernel_pid_t me = thread_getpid();

xtimer_set_wakeup(&xtimer, 1<<31, me);
Copy link
Member

Choose a reason for hiding this comment

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

fails on avr8,
change to 1ul<<31 or some variant of ((uint32_t)1 << 31)

@kaspar030 kaspar030 removed the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Aug 24, 2015
@kaspar030
Copy link
Contributor Author

Apart from squashing, travis seems happy.

@kaspar030
Copy link
Contributor Author

  • squashed

@kaspar030 kaspar030 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 25, 2015
@OlegHahm OlegHahm added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Aug 25, 2015
@jnohlgard
Copy link
Member

Feel free to review and ACK this during tonight's session, I will not be participating however. I feel like this PR has become quite mature now.

@haukepetersen
Copy link
Contributor

ACK once Travis is happy

@haukepetersen
Copy link
Contributor

kicked Travis, errors seemed to be unrelated.

@kaspar030
Copy link
Contributor Author

Travis is happy, GO!

kaspar030 added a commit that referenced this pull request Aug 25, 2015
sys: add new timer subsystem
@kaspar030 kaspar030 merged commit 6d1aab7 into RIOT-OS:master Aug 25, 2015
@kaspar030 kaspar030 deleted the wtimer branch August 25, 2015 17:20
@kaspar030
Copy link
Contributor Author

Everybody, thanks a lot for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Impact: major The PR changes a significant part of the code base. It should be reviewed carefully PR-award-nominee Deprecated. Will be removed soon. Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants