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/xtimer: _xtimer_now64(): fix irq_disable() return value type #13182

Merged
merged 1 commit into from
Jan 22, 2020

Conversation

kaspar030
Copy link
Contributor

Contribution description

Small mistake, large impact. Use of the wrong type for the return value of irq_disable() basically chopped off part of the ISR state.

On the fe310, this lead to a trap right after xtimer ISR exit, sometimes.

Testing procedure

Fix should be obvious.

Issues/PRs references

Fixes #13109.

@kaspar030 kaspar030 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: timers Area: timer subsystems labels Jan 22, 2020
@fjmolinas fjmolinas added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jan 22, 2020
@emmanuelsearch
Copy link
Member

Nice catch @kaspar030 !

@fjmolinas
Copy link
Contributor

Tested on hifive1b:

  • master
2020-01-22 12:55:05,965 # ATE0-->ATE0
2020-01-22 12:55:05,982 # OK
2020-01-22 12:55:06,140 # AT+BLEINIT=0-->OK
2020-01-22 12:55:06,295 # AT+CWMODE=0-->OK
2020-01-22 12:55:06,296 # 
2020-01-22 12:55:06,304 # Help: Press s to start test, r to print it is ready
s
2020-01-22 12:55:23,420 # START
2020-01-22 12:55:23,426 # main(): This is RIOT! (Version: 2020.04-devel-19-g97a71b-HEAD)
2020-01-22 12:55:23,430 # Running test 5 times with 7 distinct sleep times
2020-01-22 12:55:23,441 # Unhandled trap:
2020-01-22 12:55:23,443 #   mcause: 0x00000001
2020-01-22 12:55:23,444 #   mepc:   0x200104e8
2020-01-22 12:55:23,446 #   mtval:  0x200104e8
2020-01-22 12:55:23,448 # *** RIOT kernel panic:
2020-01-22 12:55:23,450 # Unhandled trap
2020-01-22 12:55:23,450 # 
2020-01-22 12:55:23,451 # *** halted.
2020-01-22 12:55:23,451 # 

-pr

Bench Clock Reset Complete
2020-01-22 12:56:06,256 # 
2020-01-22 12:56:06,305 # ATE0-->ATE0
2020-01-22 12:56:06,322 # OK
2020-01-22 12:56:06,479 # AT+BLEINIT=0-->OK
2020-01-22 12:56:06,635 # AT+CWMODE=0-->OK
2020-01-22 12:56:06,636 # 
2020-01-22 12:56:06,644 # Help: Press s to start test, r to print it is ready
s
2020-01-22 12:56:10,716 # START
2020-01-22 12:56:10,722 # main(): This is RIOT! (Version: 2020.04-devel-71-g885bb84-pr-13182)
2020-01-22 12:56:10,727 # Running test 5 times with 7 distinct sleep times
2020-01-22 12:56:10,741 # Slept for 10102 us (expected: 10000 us) Offset: 102 us
2020-01-22 12:56:10,795 # Slept for 49988 us (expected: 50000 us) Offset: -12 us
2020-01-22 12:56:10,809 # Slept for 10223 us (expected: 10234 us) Offset: -11 us
2020-01-22 12:56:10,870 # Slept for 56763 us (expected: 56780 us) Offset: -17 us
2020-01-22 12:56:10,886 # Slept for 12116 us (expected: 12122 us) Offset: -6 us
2020-01-22 12:56:10,989 # Slept for 98755 us (expected: 98765 us) Offset: -10 us
2020-01-22 12:56:11,068 # Slept for 74982 us (expected: 75000 us) Offset: -18 us
2020-01-22 12:56:11,081 # Slept for 9980 us (expected: 10000 us) Offset: -20 us
2020-01-22 12:56:11,135 # Slept for 49988 us (expected: 50000 us) Offset: -12 us
2020-01-22 12:56:11,150 # Slept for 10224 us (expected: 10234 us) Offset: -10 us
2020-01-22 12:56:11,210 # Slept for 56762 us (expected: 56780 us) Offset: -18 us
2020-01-22 12:56:11,226 # Slept for 12116 us (expected: 12122 us) Offset: -6 us
2020-01-22 12:56:11,329 # Slept for 98755 us (expected: 98765 us) Offset: -10 us
2020-01-22 12:56:11,408 # Slept for 74981 us (expected: 75000 us) Offset: -19 us

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! Thanks for investigating this and providing the fix @kaspar030, I'll run all tests on hifive1b ass good measure, but the fix is obvious.

@fjmolinas
Copy link
Contributor

I'll use the results in #13086 as a comparison point.

@fjmolinas
Copy link
Contributor

GO!, still waiting on all tests results but the change is valid on its own.

@fjmolinas
Copy link
Contributor

Backport provided in #13183

@kaspar030 kaspar030 deleted the fix_xtimer_now_irqdisable branch January 22, 2020 12:31
@aabadie
Copy link
Contributor

aabadie commented Jan 22, 2020

Nice catch @kaspar030, thanks!

@miri64
Copy link
Member

miri64 commented Jan 22, 2020

As a reminder for our offline discussion: maybe having a Coccinelle check for that in the CI is a good idea.

@fjmolinas
Copy link
Contributor

Re0ran the tests on hifive1b and got pretty much the same results as before:

ERROR:hifive1b:Tests failed: 6
Failures during test:
- [tests/cpp11_mutex](tests/cpp11_mutex/test.failed)
- [tests/gnrc_ipv6_fwd_w_sub](tests/gnrc_ipv6_fwd_w_sub/test.failed)
- [tests/pkg_utensor](tests/pkg_utensor/test.failed)
- [tests/ps_schedstatistics](tests/ps_schedstatistics/test.failed)
- [tests/xtimer_periodic_wakeup](tests/xtimer_periodic_wakeup/test.failed)
- [tests/xtimer_usleep](tests/xtimer_usleep/test.failed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fe310: xtimer hardfault
5 participants