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.c: _mutex_timeout() cleanup #11807

Conversation

JulianHolzwarth
Copy link
Contributor

Contribution description

This pr prepares for bug fixes and improvements of xtimer.c:xtimer_mutex_lock_timeout() (the tracking pr: #11660 )

This pr improves the _mutex_timeout() function.
It uses now sched_switch instead of thread_yield_higher. (#11660 (comment), #11759)
Only call sched_switch (thread_yield_higher before) when the thread was unlocked.
And ensure interrupt are disabled because it modifies a mutex. (if xtimer_set spins the callback is executed in the thread context.)

This pr also changes the comment of sys/include/xtimer.h: xtimer_mutex_lock_timeout()

Testing procedure

BOARD=native make -C tests/xtimer_mutex_lock timeout/ flash test
output:

...
> mutex_timeout_n_spin_unlocked
starting test: xtimer mutex lock timeout
OK
> mutex_timeout_n_spin_locked
mutex_timeout_n_spin_locked
starting test: xtimer mutex lock timeout
OK
> 

Issues/PRs references

the tracking pr: #11660
reason for sched_switch instead of thread_yield_higher: #11759, #11660 (comment)

removing line because core_thread_flags is not required
@JulianHolzwarth JulianHolzwarth changed the title sys/xtimer/xtimer.c: _mutex_timeout() clean ups sys/xtimer/xtimer.c: _mutex_timeout() cleanup Jul 10, 2019
@JulianHolzwarth
Copy link
Contributor Author

@vincent-d #11543 (comment) Could you look at this pr? It prepares for the change talked about in #11543

@MrKevinWeiss MrKevinWeiss added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: timers Area: timer subsystems labels Jul 18, 2019
Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

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

I think this is OK. Squash.

@jcarrano
Copy link
Contributor

jcarrano commented Aug 9, 2019

I mean, squash the comment only.

if xtimer_set spins the callback is executed in the thread context.

comment to explain irq_disable
and when this line could be removed
(when xtimer stops executing the callback funtion from thread context)
Only yields and change threads status when thread was removed from mutex list.
because of pr RIOT-OS#11759: not all boards check for is_in_irq when thread_yield_higher
@JulianHolzwarth JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/first_fix_improvements branch from e08fa67 to c1f5818 Compare August 9, 2019 15:31
@JulianHolzwarth
Copy link
Contributor Author

Ok. Done.

@jcarrano jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 9, 2019
@jcarrano jcarrano merged commit b48afd6 into RIOT-OS:master Aug 9, 2019
@JulianHolzwarth JulianHolzwarth deleted the pr/xtimer_mutex_lock_timeout/first_fix_improvements branch August 9, 2019 17:39
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants