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

core: hwtimer: use disable/restoreIRQ #1883

Merged
merged 1 commit into from
Oct 28, 2014
Merged

core: hwtimer: use disable/restoreIRQ #1883

merged 1 commit into from
Oct 28, 2014

Conversation

kaspar030
Copy link
Contributor

hwtimer still uses dINT/eINT(). This PR makes it use disableIRQ/restoreIRQ instead.

@kaspar030 kaspar030 added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Oct 27, 2014
LudwigKnuepfer
LudwigKnuepfer reviewed Oct 27, 2014
View reviewed changes
core/hwtimer.c
if (!inISR()) {
dINT();
state = disableIRQ();
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the if (!inISR()) be unnecessary?

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 wasn't sure if it is on all platforms.

I'd say, this is an improvement, removing the if(!inISR()) can be a further optimization. Let's not mix both and see if the restoreIRQ is fine, then remove the inISR()s at a later time.

Copy link
Member

Choose a reason for hiding this comment

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

fine with me.

@LudwigKnuepfer LudwigKnuepfer mentioned this pull request Oct 27, 2014
Closed
@kaspar030
Copy link
Contributor Author

Wierd. Looks to me that at some point all timers shoot at once, causing a hwtimer for the tick to be set, which shoots at once too. There's some corruption going on somewhere...

@LudwigKnuepfer
Copy link
Member

@kaspar030 on which platform, application?

@kaspar030
Copy link
Contributor Author

@LudwigOrtmann Sorry, I was tired and wanted to comment the vtimer bug. This one is fine in all tests.

This was referenced Oct 28, 2014
Closed
Closed
@benpicco
Copy link
Contributor

ACK and go I guess?

benpicco added a commit that referenced this pull request Oct 28, 2014
@benpicco
Merge pull request #1883 from kaspar030/hwtimer_new_irq_api
37d8cab 
core: hwtimer: use disable/restoreIRQ
@benpicco benpicco merged commit 37d8cab into RIOT-OS:master Oct 28, 2014
@OlegHahm
Copy link
Member

Second ACK was missing for core change.

@OlegHahm
Copy link
Member

This PR creates warning. Fixed in #1901.

@kaspar030 kaspar030 deleted the hwtimer_new_irq_api branch January 26, 2017 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@benpicco benpicco

Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! 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.
4 participants
@kaspar030 @LudwigKnuepfer @benpicco @OlegHahm