Skip to content

sys: xtimer concurrency/robustness improvement #9530

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
Merged
merged 11 commits into from
Jan 13, 2020
Merged

sys: xtimer concurrency/robustness improvement #9530

merged 11 commits into from
Jan 13, 2020

Conversation

Hyungsin
Copy link

@Hyungsin Hyungsin commented Jul 9, 2018 •
edited
Loading

Contribution description

This PR tries to resolve concurrency/robustness problem of xtimer, an attempt to escape from long suffering of xtimer's unrobustness. I personally have experienced frequency system crash due to xtimer. As an embedded OS, RIOT can have a little timing error but must avoid system crash to enable long-term attended operation of embedded devices.

  1. All time-sensitive sections are interrupt free.
  2. Support various hardware timers including <32 bits
  3. Support overflow

The basic idea is to change xtimer operation from "absolute target-based" to "period-based", which is the case of TinyOS. Timer expiry and current time update are based on "offset" rather than "absolute target". An absolute target value is used only for sorting timers chronologically and setting a lltimer. I personally tested this PR with a Hamilton board and xtimer test applications in 'tests' folder. But further test might be needed.

I'm aware of other ongoing effort for fixing xtimer. I'm just throwing another option here since I have some time to focus on this issue now, and leave the decision whether to merge this PR with reviewers. @kaspar030, @gebart, This PR is the one I mentioned earlier about xtimer.

The next step would be reworking on #7053, with a simpler implementation.

Issues/PRs references

#5428, #6937, #9211, #9491

@Hyungsin Hyungsin mentioned this pull request Jul 9, 2018
Closed
@Hyungsin Hyungsin force-pushed the forupstream_xtimer branch 8 times, most recently from d4dacd5 to 019f915 Compare July 10, 2018 01:10
@PeterKietzmann PeterKietzmann added the Area: timers Area: timer subsystems label Jul 10, 2018
@Hyungsin Hyungsin force-pushed the forupstream_xtimer branch 6 times, most recently from 27219b7 to caf66f3 Compare July 11, 2018 21:51
@Hyungsin Hyungsin force-pushed the forupstream_xtimer branch 6 times, most recently from 7e7a4ac to 9548ec4 Compare August 8, 2018 06:48
@Hyungsin Hyungsin closed this Aug 8, 2018
@Hyungsin Hyungsin force-pushed the forupstream_xtimer branch from 9548ec4 to 3f3df74 Compare August 8, 2018 06:53
@Hyungsin Hyungsin reopened this Aug 8, 2018
@Hyungsin Hyungsin force-pushed the forupstream_xtimer branch 5 times, most recently from 489985c to a672186 Compare August 8, 2018 07:25
@Hyungsin
Copy link
Author

Hyungsin commented Jan 11, 2020 •
edited
Loading

All checks have passed.

@fjmolinas
Copy link
Contributor

@MichelRottleuthner so would you say this is ready now?

MichelRottleuthner
MichelRottleuthner approved these changes Jan 12, 2020
View reviewed changes
Copy link
Contributor

@MichelRottleuthner MichelRottleuthner left a comment

Choose a reason for hiding this comment

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

@MichelRottleuthner so would you say this is ready now?

Yes let's finally do this. I give my ACK.
Thank you all for helping!
We should still have a close look and watch out very carefully for any suspicious behavior that could be related to regressions or hidden bugs in the upcoming release tests.

fjmolinas
fjmolinas approved these changes Jan 12, 2020
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK!

@fjmolinas
Copy link
Contributor

We should still have a close look and watch out very carefully for any suspicious behavior that could be related to regressions or hidden bugs in the upcoming release tests.

For sure. Thanks for all the work and patience @Hyungsin!

@fjmolinas
Copy link
Contributor

@MichelRottleuthner I let you pull trigger!

@MichelRottleuthner MichelRottleuthner merged commit 53cb3b0 into RIOT-OS:master Jan 13, 2020
@kaspar030 kaspar030 mentioned this pull request Jan 13, 2020
Closed
This was referenced Jan 13, 2020
Merged
Merged
@fjmolinas fjmolinas mentioned this pull request Jan 13, 2020
Closed
This was referenced Jan 13, 2020
Merged
Merged
@fjmolinas fjmolinas mentioned this pull request Jan 22, 2020
Closed
@MichelRottleuthner MichelRottleuthner mentioned this pull request Jan 22, 2020
Closed
@JulianHolzwarth
Copy link
Contributor

JulianHolzwarth commented Jan 22, 2020 •
edited
Loading

@MichelRottleuthner this PR removed #11992
This should not be the case! #11992 is an important bug fix.

@MichelRottleuthner
Copy link
Contributor

Can you file a PR for that again? We need a proper automated regression test for that.

@JulianHolzwarth JulianHolzwarth mentioned this pull request Jan 22, 2020
Merged
This was referenced Jan 24, 2020
Closed
Merged
@aabadie aabadie mentioned this pull request Jan 27, 2020
Closed
@Hyungsin Hyungsin mentioned this pull request Jan 27, 2020
Merged
@MichelRottleuthner MichelRottleuthner mentioned this pull request Jan 30, 2020
Merged
This was referenced Feb 11, 2020
Closed
Closed
@kaspar030 kaspar030 mentioned this pull request Apr 9, 2020
Merged
@pokgak pokgak mentioned this pull request Jul 2, 2020
Closed
This was referenced Jul 2, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaspar030 kaspar030 kaspar030 left review comments

@MichelRottleuthner MichelRottleuthner MichelRottleuthner approved these changes

@fjmolinas fjmolinas fjmolinas approved these changes

@jnohlgard jnohlgard Awaiting requested review from jnohlgard

@maribu maribu Awaiting requested review from maribu

+1 more reviewer

@JulianHolzwarth JulianHolzwarth JulianHolzwarth left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@jnohlgard jnohlgard

Labels
Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2020.01
Development

Successfully merging this pull request may close these issues.