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.

Sign up for GitHub

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

Jump to bottom

sys: make xtimer isr-safe #5428

Closed
wants to merge 5 commits into from
Closed

sys: make xtimer isr-safe #5428

wants to merge 5 commits into from

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented May 10, 2016 •
edited
Loading

I started making xtimer ISR-safe, ended up refactoring and simplifying a lot.

Changes:

  • xtimer now "ticks" twice per timer period instead of once
  • _long_cnt and _high_cnt are now based on half periods, share one bit with the actual timer's MSB
  • xtimer_now() and xtimer_now64() use bit fiddling to compensate for a missed timer tick
  • dropped overflow_list and long_list. all timers end up in the same.
  • xtimer_set* now all use _xtimer_set64()
  • now that we're using half-ticks, it is possible to detect underflows previously causing a hang -> timers setting a time in the past (using _xtimer_set_absolute() or by underflows) get triggered right away

Marked as API change as xtimer_usleep_until() now takes a 64bit state argument.
Marked WIP as it has only seen brief testing on native, samr21, msb-430h.

What do you think?

Fixes #4902.

@kaspar030 kaspar030 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) 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 Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: timers Area: timer subsystems labels May 10, 2016
@OlegHahm
Copy link
Member

I can test this on iotlab-m3 since my application seems to suffer from the bug.

@kaspar030
Copy link
Contributor Author

I can test this on iotlab-m3 since my application seems to suffer from the bug.

That would be nice, especially as the iotlab-m3 has 16bit timers...

@kaspar030 kaspar030 mentioned this pull request May 10, 2016
Closed
@kaspar030
Copy link
Contributor Author

For easier reviewing, 7dc9ed2 contains the main changes, the rest is cosmetic.

lebrush
lebrush reviewed May 11, 2016
View reviewed changes
sys/xtimer/xtimer_core.c
#include "debug.h"

static volatile int _in_handler = 0;
static int _in_handler = 0;
Copy link
Member

Choose a reason for hiding this comment

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

why not volatile anymore?

Copy link
Member

Choose a reason for hiding this comment

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

Why not use atomic_int_t anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured it is only changed to one, possibly read, changed to zero within the handler ISR. Outside it is only read and then should always be zero. So volatile doesn't protect anything. More atomic access is not needed.

@malosek malosek mentioned this pull request May 11, 2016
Closed
@kaspar030
Copy link
Contributor Author

I can test this on iotlab-m3 since my application seems to suffer from the bug.

did you? ;)

@OlegHahm
Copy link
Member

Oops, sorry, forgot, will do ASAP.

@kYc0o kYc0o added this to the Release 2016.07 milestone May 30, 2016
@kaspar030
Copy link
Contributor Author

  • addressed @lebrush's comment
  • added some comments on the used logic. please check. ;)

@OlegHahm
Copy link
Member

I still have xtimer problems - with and without this patch, the result looks similar. I have a timer running that should tick every second, but the result looks like:

20160531-11.40.07.7307 m3-138.lille.iot-lab.info: cluster: SECOND: 46
20160531-11.40.07.7589 m3-138.lille.iot-lab.info: cluster: SECOND: 45
20160531-11.40.07.7861 m3-138.lille.iot-lab.info: cluster: SECOND: 44
20160531-11.40.08.6489 m3-138.lille.iot-lab.info: cluster: SECOND: 43
20160531-11.40.08.6795 m3-138.lille.iot-lab.info: cluster: SECOND: 42
20160531-11.40.08.7592 m3-138.lille.iot-lab.info: > cluster: SECOND: 41
20160531-11.40.08.7839 m3-138.lille.iot-lab.info: cluster: SECOND: 40
20160531-11.40.09.3328 m3-138.lille.iot-lab.info: cluster: SECOND: 39
20160531-11.40.09.4122 m3-138.lille.iot-lab.info: cluster: SECOND: 38
20160531-11.40.09.4340 m3-138.lille.iot-lab.info: cluster: SECOND: 37
20160531-11.40.09.5055 m3-138.lille.iot-lab.info: > cluster: SECOND: 36
20160531-11.40.15.9163 m3-138.lille.iot-lab.info: cluster: SECOND: 35
20160531-11.40.16.7206 m3-138.lille.iot-lab.info: cluster: SECOND: 34
20160531-11.40.25.4697 m3-138.lille.iot-lab.info: cluster: SECOND: 33
20160531-11.40.34.6166 m3-138.lille.iot-lab.info: cluster: SECOND: 32
20160531-11.40.43.4992 m3-138.lille.iot-lab.info: cluster: SECOND: 31
20160531-11.40.52.7583 m3-138.lille.iot-lab.info: > cluster: SECOND: 30
20160531-11.41.01.4963 m3-138.lille.iot-lab.info: cluster: SECOND: 29
20160531-11.41.13.7803 m3-138.lille.iot-lab.info: cluster: SECOND: 28
20160531-11.41.19.3856 m3-138.lille.iot-lab.info: cluster: SECOND: 27
20160531-11.41.27.5412 m3-138.lille.iot-lab.info: cluster: SECOND: 26
20160531-11.41.36.1397 m3-138.lille.iot-lab.info: cluster: SECOND: 25
20160531-11.41.42.5770 m3-138.lille.iot-lab.info: cluster: SECOND: 24
20160531-11.41.46.6082 m3-138.lille.iot-lab.info: cluster: SECOND: 23

@kaspar030
Copy link
Contributor Author

@OlegHahm Hm. Maybe I can take a look at your code?

@OlegHahm
Copy link
Member

Sure you can, but it's a mess: https://github.com/OlegHahm/miniature-dangerzone/tree/master/ccnl_clustering

I'm trying to create a simpler example that shows the problem.

@kaspar030
Copy link
Contributor Author

kaspar030 commented May 31, 2016 via email

@OlegHahm
Copy link
Member

I'm trying.

@OlegHahm
Copy link
Member

sample output:

20160531-15.18.15.5718 m3-196.lille.iot-lab.info: 15954087 cluster: SET SECOND TIMER
20160531-15.18.16.5531 m3-196.lille.iot-lab.info: cluster: SECOND: 86
20160531-15.18.16.5704 m3-196.lille.iot-lab.info: 16955271 cluster: SET SECOND TIMER
20160531-15.18.17.5539 m3-196.lille.iot-lab.info: cluster: SECOND: 85
20160531-15.18.17.5715 m3-196.lille.iot-lab.info: 17956455 cluster: SET SECOND TIMER
20160531-15.18.18.5553 m3-196.lille.iot-lab.info: cluster: SECOND: 84
20160531-15.18.18.5698 m3-196.lille.iot-lab.info: 18957639 cluster: SET SECOND TIMER
20160531-15.18.20.0160 m3-196.lille.iot-lab.info: cluster: SECOND: 83
20160531-15.18.20.0203 m3-196.lille.iot-lab.info: 20357249 cluster: SET SECOND TIMER
20160531-15.18.20.5967 m3-196.lille.iot-lab.info: > cluster: SECOND: 82
20160531-15.18.20.9596 m3-196.lille.iot-lab.info: 21358433 cluster: SET SECOND TIMER
20160531-15.18.21.9523 m3-196.lille.iot-lab.info: cluster: SECOND: 81
20160531-15.18.21.9576 m3-196.lille.iot-lab.info: 22359617 cluster: SET SECOND TIMER
20160531-15.18.22.9543 m3-196.lille.iot-lab.info: cluster: SECOND: 80
20160531-15.18.22.9572 m3-196.lille.iot-lab.info: 23360801 cluster: SET SECOND TIMER
20160531-15.18.24.6086 m3-196.lille.iot-lab.info: cluster: SECOND: 79
20160531-15.18.24.6187 m3-196.lille.iot-lab.info: 24361985 cluster: SET SECOND TIMER
20160531-15.18.25.7414 m3-196.lille.iot-lab.info: cluster: SECOND: 78
20160531-15.18.25.7448 m3-196.lille.iot-lab.info: 25363169 cluster: SET SECOND TIMER
20160531-15.18.25.9575 m3-196.lille.iot-lab.info: cluster: SECOND: 77
20160531-15.18.25.9614 m3-196.lille.iot-lab.info: 26364353 cluster: SET SECOND TIMER
20160531-15.18.27.2238 m3-196.lille.iot-lab.info: cluster: SECOND: 76
20160531-15.18.27.2333 m3-196.lille.iot-lab.info: 27365537 cluster: SET SECOND TIMER
20160531-15.18.29.3674 m3-196.lille.iot-lab.info: cluster: SECOND: 75
20160531-15.18.29.3728 m3-196.lille.iot-lab.info: 28369749 cluster: SET SECOND TIMER
20160531-15.18.29.9917 m3-196.lille.iot-lab.info: cluster: SECOND: 74
20160531-15.18.29.9952 m3-196.lille.iot-lab.info: 29381985 cluster: SET SECOND TIMER
20160531-15.18.30.1489 m3-196.lille.iot-lab.info: cluster: SECOND: 73
20160531-15.18.30.1523 m3-196.lille.iot-lab.info: 30383169 cluster: SET SECOND TIMER
20160531-15.18.32.1824 m3-196.lille.iot-lab.info: cluster: SECOND: 72
20160531-15.18.32.1911 m3-196.lille.iot-lab.info: 31384353 cluster: SET SECOND TIMER
20160531-15.18.33.8856 m3-196.lille.iot-lab.info: cluster: SECOND: 71
20160531-15.18.33.8899 m3-196.lille.iot-lab.info: 32385537 cluster: SET SECOND TIMER
20160531-15.18.34.9457 m3-196.lille.iot-lab.info: cluster: SECOND: 70
20160531-15.18.34.9653 m3-196.lille.iot-lab.info: 33386721 cluster: SET SECOND TIMER
20160531-15.18.35.8868 m3-196.lille.iot-lab.info: cluster: SECOND: 69

@OlegHahm
Copy link
Member

Sometimes it's even worse:

20160531-16.00.53.4481 m3-196.lille.iot-lab.info: 22123858 cluster: SET SECOND TIMER
20160531-16.00.54.1730 m3-196.lille.iot-lab.info: cluster: SECOND: 80
20160531-16.00.54.1854 m3-196.lille.iot-lab.info: 23125038 cluster: SET SECOND TIMER
20160531-16.01.00.0592 m3-196.lille.iot-lab.info: cluster: SECOND: 79
20160531-16.01.00.0653 m3-196.lille.iot-lab.info: 24127086 cluster: SET SECOND TIMER
20160531-16.01.07.2414 m3-196.lille.iot-lab.info: cluster: SECOND: 78
20160531-16.01.07.2473 m3-196.lille.iot-lab.info: 25157590 cluster: SET SECOND TIMER
20160531-16.01.09.7767 m3-196.lille.iot-lab.info: cluster: SECOND: 77
20160531-16.01.09.7823 m3-196.lille.iot-lab.info: 26158770 cluster: SET SECOND TIMER
20160531-16.01.10.5253 m3-196.lille.iot-lab.info: cluster: SECOND: 76

@kYc0o
Copy link
Contributor

kYc0o commented May 31, 2016

I'm having issues with atmega timers and I merged this to see if it gets better, but I'm experiencing more unstability... especially on xtimer_usleep_until but also in other tests.

Actually it seems that XTIMER_SHIFT() is not working, but I'm trying to get more details.

Tested in arduino-mega2560 with XTIMER_SHIFT(4) and XTIMER_SHIFT(6)

@kaspar030
Copy link
Contributor Author

@kYc0o Could you test #5451 (without this PR)? Does it improve things?

@kYc0o
Copy link
Contributor

kYc0o commented May 31, 2016

Actually xtimer is too big for arduino-uno. We need to provide some kind of native access to timers for this platform...

text       data     bss     dec     hex filename
  13100    1570    1613   16283    3f9b /Users/facosta/git/RIOT-OS/RIOT/tests/xtimer_drift/bin/arduino-uno/xtimer_drift.elf

@kaspar030
Copy link
Contributor Author

Actually xtimer is too big for arduino-uno. We need to provide some kind of native access to timers for this platform...

Oh, I meant, test it on the arduino-mega2560. ;) Somehow xtimer works better for me.

@kYc0o
Copy link
Contributor

kYc0o commented May 31, 2016

Oh yeah, I tested on mega2560 without this PR and it works well, and with a shift of 6 works even better, but xtimer_usleep_until does not work with any XTIMER_SHIFT... can you test it?

@miri64 miri64 modified the milestones: Release 2017.01, Release 2016.10 Oct 31, 2016
@OlegHahm
Copy link
Member

@kaspar030, are these problems still existing?

@kaspar030
Copy link
Contributor Author

are these problems still existing?

Yes, won't have time for this before the release.

@PeterKietzmann PeterKietzmann modified the milestones: Release 2017.04, Release 2017.01 Jan 16, 2017
@kYc0o
Copy link
Contributor

kYc0o commented Feb 8, 2017

Wow, this PR got very outdated. Is still valid @kaspar030 ?

@smlng
Copy link
Member

smlng commented Feb 10, 2017

if it fixes #4902, its worth reconsidering and working on

@smlng smlng removed this from the Release 2018.01 milestone Jan 12, 2018
@Winkelkatze Winkelkatze mentioned this pull request Jan 20, 2018
Closed
@tcschmidt
Copy link
Member

@kaspar030 does this help fixing xtimer? Then it should have highest priority, I guess ...

@Hyungsin Hyungsin mentioned this pull request Jul 9, 2018
Merged
@stale
Copy link

stale bot commented Aug 10, 2019

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 Aug 10, 2019
@kaspar030
Copy link
Contributor Author

I'll not be working on this, but concentrate on #11874.

@kaspar030 kaspar030 closed this Aug 19, 2019
@miri64
Copy link
Member

miri64 commented Aug 27, 2019

Nevertheless, this issue should be kept open as a note, until ztimer fully replaces xtimer

@miri64 miri64 reopened this Aug 27, 2019
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 27, 2019
@kaspar030
Copy link
Contributor Author

Obsoleted by #9530.

@kaspar030 kaspar030 closed this Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jnohlgard jnohlgard

Labels
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: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. State: waiting for other PR State: The PR requires another PR to be merged first State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

xtimer: xtimer_set
10 participants
@kaspar030 @OlegHahm @kYc0o @malosek @jnohlgard @miri64 @smlng @tcschmidt @lebrush @PeterKietzmann