Skip to content

cpu/arm7_common: Fix thread_yield_higher in ISR#11887

Merged
benpicco merged 1 commit intoRIOT-OS:masterfrom
maribu:arm7_thread_yield_higher
Sep 11, 2019
Merged

cpu/arm7_common: Fix thread_yield_higher in ISR#11887
benpicco merged 1 commit intoRIOT-OS:masterfrom
maribu:arm7_thread_yield_higher

Conversation

@maribu
Copy link
Member

@maribu maribu commented Jul 23, 2019

Contribution description

thread_yield_higher() in interrupt context must not yield immediately, but at the end of the ISR. This PR fixes the behavior.

Testing procedure

Issues/PRs references

Addresses the issue identified in #11759

thread_yield_higher() in interrupt context must not yield immediately, but at
the end of the ISR. This commit fixes the behavior.
@maribu maribu added Platform: ARM Platform: This PR/issue effects ARM-based platforms 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 labels Jul 23, 2019
@maribu
Copy link
Member Author

maribu commented Jul 23, 2019

Maybe @cladmi can have a short look and maybe @kaspar030 or @miri64 could review?

@cladmi
Copy link
Contributor

cladmi commented Jul 23, 2019

To complete on the comments on #11759 and #11883 (comment)

thread_yield_higher is called from interrupt context in xtimer https://github.com/RIOT-OS/RIOT/blob/master/sys/xtimer/xtimer.c#L240-L252 since #6158 (sometime it is not an interrupt context if the function spins).

thread_yield is called from interrupt context in tests/thread_float since #6630.
And does not have alternative to be called from interrupt context (as thread_yield_higher has.

Boards have specific handling for thread_yield_higher from interrupt #11759 (comment)

For me, it has become de-facto necessary that these functions must be callable from interrupt context.
It makes sense to currently handle it in the cpu.
There could be a change to remove this but it would currently be a regression and maybe break existing code.

This change now makes tests/thread_float/ correctly switch threads (even if the printed float is always 0.0 but is unrelated to this).

BOARD=msba2 make --no-print-directory -C tests/thread_float/ flash term
...
2019-07-23 15:15:57,017 - INFO # main(): This is RIOT! (Version: 2019.10-devel-110-gfbf42-pr/riot/11887/Fix_thread_yield_higher_in_ISR)
2019-07-23 15:15:57,018 - INFO # THREADS CREATED
2019-07-23 15:15:57,018 - INFO #
2019-07-23 15:15:57,020 - INFO # THRTHRTHREEADEAD AD  5 s5 s5 sttartartartt
2019-07-23 15:15:57,020 - INFO #
2019-07-23 15:15:57,021 - INFO #
2019-07-23 15:15:57,021 - INFO # T(5): 0.000000
2019-07-23 15:15:57,022 - INFO # T(3): 0.000000
2019-07-23 15:15:57,023 - INFO # T(5): 0.000000
2019-07-23 15:15:57,024 - INFO # T(3): 0.000000
2019-07-23 15:15:57,071 - INFO # T(5): 0.000000

(the mixed output at the beginning is also here on samr21-xpro).

The test from #11759 also works with this PR

BOARD=msba2 make --no-print-directory -C tests/isr_thread_yield_higher/ flash test
...
2019-07-23 15:14:53,007 - INFO # main(): This is RIOT! (Version: 2019.10-devel-110-gfbf42-pr/riot/11887/Fix_thread_yield_higher_in_ISR)
2019-07-23 15:14:53,008 - INFO # Send character to start
2019-07-23 15:14:54,948 - INFO # Go to sleep
2019-07-23 15:14:55,941 - INFO # Value is 1

@maribu
Copy link
Member Author

maribu commented Jul 26, 2019

@kaspar030, @miri64, @PeterKietzmann: Could one of you review this PR?

@miri64
Copy link
Member

miri64 commented Jul 26, 2019

I did not really follow the discussion around thread_yield_higher() so I only can test, but not really review this.

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

I don't have hardware to test, but the change is easy to follow nonetheless.
sched_context_switch_request is evaluated by arm_irq_handler right after the ISR (which is the only piece of code that could call thread_yield_higher() in interrupt context) has been executed.

When not in interrupt context, a Supervisor Call is executed as before.

@cladmi
Copy link
Contributor

cladmi commented Sep 11, 2019

The test from #11759 is now working in master.

a
/srv/ilab-builds/workspace/jobs/experimental-pull-request-tests/dist/tools/pyterm/pyterm -p "/dev/riot/ttyMSBA2" -b "115200" -tg --noprefix --no-repeat-command-on-empty-line
Twisted not available, please install it if you want to use pyterm's JSON capabilities
Connect to serial port /dev/riot/ttyMSBA2
Welcome to pyterm!
Type '/exit' to exit.
main(): This is RIOT! (Version: buildtest)
Send character to start
Go to sleep
Value is 1

I updated the PR description there.

Thank you for handling it.

@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
@maribu maribu deleted the arm7_thread_yield_higher branch November 4, 2019 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms 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.

5 participants