tests/xtimer_mutex_lock_timeout: New test by JulianHolzwarth · Pull Request #12008 · RIOT-OS/RIOT

JulianHolzwarth · 2019-08-14T14:49:13Z

Tracking: pr #11660

Contribution description

Implements a new function for xtimer_mutex_lock_timeout to make sure the function works with threads. The new function tests xtimer_mutex_lock_timeout by locking the mutex from another thread, calling xtimer_mutex_lock_timeout and unlocking the mutex from the other thread before the timeout ends.

(The test also shows that the created thread exits/ends)

The test also prints a empty line after each test function to make it easier to read and removes a wrong include.

Testing procedure

BOARD=native make -C tests/xtimer_mutex_lock_timeout/ flash test
it outputs:

...
> mutex_timeout_long_unlocked
mutex_timeout_long_unlocked
starting test: xtimer mutex lock timeout
OK

> mutex_timeout_long_locked
mutex_timeout_long_locked
starting test: xtimer mutex lock timeout
OK

> mutex_timeout_long_locked_low
mutex_timeout_long_locked_low
starting test: xtimer mutex lock timeout with thread
threads = 2
THREAD low prio: start
MAIN THREAD: calling xtimer_mutex_lock_timeout
OK
threads = 3
MAIN THREAD: waiting for created thread to end
THREAD low prio: exiting low
threads = 2

>

Issues/PRs references

tracking #11660

MichelRottleuthner

Thanks for working on this! I added some comments inline. One general remark: would it make sense to also check if the timeout is actually working in the low-prio case? I.e a case where the timeout is actually triggered (maybe even with a check to see if the timing is correct)?

tests/xtimer_mutex_lock_timeout/main.c

MichelRottleuthner · 2019-08-23T15:06:46Z

tests/xtimer_mutex_lock_timeout/main.c

+
+    mutex_lock(test_mutex);
+    thread_wakeup(main_thread_pid);
+    while (msg_try_receive(&msg) == -1) {}


first: why not just blocking call of msg_receive instead of this loop? second: why use msg send/recv here at all? shouldn't the thread_sleep() + thread_wakeup() be enough to lock the mutex in the low prio thread before the main thread continues?

after thread wakeup the main thread runs anyways. I don't need that line it is only to better understand what is happening (the thread waits but is still runnable)
The thread should only check receive once.
I can remove it if it is more confusing than helpful.

MichelRottleuthner · 2019-08-23T15:08:27Z

tests/xtimer_mutex_lock_timeout/main.c

+    mutex_unlock(test_mutex);
+    (void)irq_disable();
+    puts("THREAD low prio: exiting low");
+    msg_send_int(&msg, main_thread_pid);


why msg_send_int? This is clearly not called from interrupt context

to join threads. Only way to wait for a thread to finish that has a lower prio as far as I know(is done this way in posix reaper implementation)

msg_send_int does not yield but still unblocks the other thread.

I will split it in a function with documentation

I added a function for it.

tests/xtimer_mutex_lock_timeout/main.c

JulianHolzwarth · 2019-08-23T16:22:10Z

There are more tests I want that are currently failing. tracking #11660
Checking timing should be more done for xtimer_set64 than this one.
Checking if timeout is triggering is tested in cmd_test_xtimer_mutex_lock_timeout_long_locked. (Mutex does not check the owner so locking it in another thread and calling xtimer_mutex_lock_timeout should not make any difference)

JulianHolzwarth · 2019-08-23T17:03:20Z

I want to rebase this pr before it gets merged.

JulianHolzwarth · 2019-08-28T14:02:30Z

@MichelRottleuthner What do you think after the changes and comments I made?

JulianHolzwarth · 2019-09-25T14:26:20Z

@MichelRottleuthner Could you look at this? What do you think after the changes and comments I made?

MichelRottleuthner

Thanks for keep bugging me.. and sorry for the delay ;) The changes you made are fine. But I still found some documentation things, see below. After that it should be ready for merge, so feel free to already suqash everything together as needed since the changes are small enough to still keep track.

tests/xtimer_mutex_lock_timeout/main.c

MichelRottleuthner · 2019-09-25T15:42:46Z

tests/xtimer_mutex_lock_timeout/main.c

    { "mutex_timeout_long_locked", "locked mutex with long timeout",
      cmd_test_xtimer_mutex_lock_timeout_long_locked, },
+    { "mutex_timeout_long_locked_low",
+      "locked mutex from lower prio thread function with long timeout",


actually the description part of the command is confusing as its not really explaining what happens here. Somewhere in the test it should be explained in clear to understand language what the test checks, what kind of guarantee this implies and by that what it means if the test fails. Maybe for the short description in the command something like "checks if xtimer_mutex_lock_timeout can acquire a mutex that was previously locked by a lower prio thread" would be better to understand for a random dude looking at this test for the first time. Also the terms low and long are not very precise here. I guess you want to express something like longer than needed i.e. the timeout is big enough that we are absolutely sure the timeout should never actually be triggered if everything is working fine. And too short as in "the timeout is so small that we know it should always trigger if everything works.

The description should be something short because a long one is difficult to read. If a "random dude" wants to know what the test does he should read the documentation. I would almost say the command and description is currently too long. I agree that the description is currently not perfect. What do you think about locked mutex with long timeout tested with 2 threads
The part: locked mutex with long timeout is to be consistent with the other commands.

LONG means the timer does not spin(timeout longer than XTIMER_BACKOFF).
NOT that is does not trigger.
It should trigger in cmd_test_xtimer_mutex_lock_timeout_long_locked.
It is only called LONG because there will be a test with a short one (meaning the timer will spin). This test will be added as soon as the bugs are removed because it fails at the moment.

tracking pr to show what my goal is (how I will remove the bugs)#11660

LOW and LONG are not meant to be precise. It is only short for lower than main thread and longer than XTIMER_BACKOFF. How much longer/lower is irrelevant for the test.

To sum it up for me a help description should be short. I am not against changing the description. And long and low are chosen on purpose.

Ok thanks for the clarification. After understanding your intentions for the naming (long) I still think its not precise. I.e. as a reader you don't get the relevant information - that is "no-spin", how about using that instead?.

To sum it up for me a help description should be short.

I agree. but it should also be as clear as possible because otherwise its no help ;) How about lock low-prio-locked-mutex from high-prio-thread (no-spin timeout)

(the dashes are meant to make it more clear which word modifies what)

mutex_timeout_long_locked_low lock low-prio-locked-mutex from high-prio-thread (no-spin timeout) is a little difficult to read

I changed it.

True - but if you know where the description starts its not really worse than before. and at least it more unambiguous...

Would you also change the "long" part of the other descriptions to "no-spin" to make it consistent again?

JulianHolzwarth · 2019-09-25T15:53:09Z

Working on it Friday.

MichelRottleuthner · 2019-09-27T15:20:52Z

please also have a look at the commit message restrictions that Travis is complaining about

JulianHolzwarth · 2019-09-27T15:23:53Z

please also have a look at the commit message restrictions that Travis is complaining about

JulianHolzwarth · 2019-09-27T15:24:16Z

missclick wanted to comment.

JulianHolzwarth · 2019-09-27T15:38:18Z

please also have a look at the commit message restrictions that Travis is complaining about

done

MichelRottleuthner · 2019-10-01T08:34:40Z

Hmm seems the test is now too big for some arduino boards and a small nucleo. Do you think we can shrink it down a bit? I think it would preferable to have it working on as many platforms as possible.
Some things that we could try to reduce code size:

comment out the ps and shell_commands module and adding a note that this can be optionally enabled for better manual testing (its not required for the automatic test, right?).
Remove prints that are not absolutely required, and shrink down those that are. E.g. the string of the test commands itself can be reduced to something like "test1", "test2" etc. since the description already carries all the required information; it also makes reading the shell help easier..
maybe reuse strings used in multiple places.
reduce stacksize of thread_low_prio_test

JulianHolzwarth · 2019-10-02T14:10:29Z

I added the same BOARD_INSUFFICIENT_MEMORY as the shell test does and reduced memory.

JulianHolzwarth · 2019-10-02T15:04:05Z

Travis fails because of fixups. (rebase after @MichelRottleuthner sees my changes)
Same for Murdock

MichelRottleuthner · 2019-10-07T08:05:13Z

OK I looked thru the new fixup commits. Feel free to squash and rebase.

printing emty line after each test function to make the test easier to read

Two Functions cmd_test_xtimer_mutex_lock_timeout_low_prio_thread and thread_low_prio_test are added. This testfunction will test xtimer_mutex_lock_timeout with two threads (main thread and lower prio than main thread). The main thread creates another thread and sleeps. While the main thread sleeps the other thread takes the mutex and wakes the main thread up. Then the main thread calls xtimer_mutex_lock_timeout and the second thread unlocks the mutex and the main thread gets it and waits for the created thread to end. Has test messages showing the thread count. To make sure the created thread ends. (test messages may be removed in the future)

The function will terminate the thread and send the message m to target_pid.

"(no-spin timeout)" instead of "long timeout" to make it consistent

JulianHolzwarth · 2019-10-11T13:49:20Z

OK I looked thru the new fixup commits. Feel free to squash and rebase.

Ok. Done.

MichelRottleuthner

All requests were addressed, testing on native and nucleo-l476rg showed no problems for me, code looks fine -> ACK

JulianHolzwarth mentioned this pull request Aug 14, 2019

[TRACKING] xtimer/xtimer.c: xtimer_mutex_lock_timeout fix and improvements #11660

Merged

6 tasks

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/new_test branch from 1b48933 to 98de62e Compare August 14, 2019 15:07

jcarrano added Area: tests Area: tests and testing framework Area: timers Area: timer subsystems labels Aug 14, 2019

tcschmidt requested a review from MichelRottleuthner August 17, 2019 00:09

MichelRottleuthner requested changes Aug 23, 2019

View reviewed changes

MichelRottleuthner requested changes Sep 25, 2019

View reviewed changes

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/new_test branch 2 times, most recently from 0c14824 to b90b7ab Compare September 27, 2019 15:09

JulianHolzwarth closed this Sep 27, 2019

JulianHolzwarth reopened this Sep 27, 2019

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/new_test branch from 62f6e8e to 6566051 Compare September 27, 2019 15:31

MichelRottleuthner added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 27, 2019

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/new_test branch from 754b768 to 8e6bb25 Compare October 2, 2019 14:16

JulianHolzwarth added 6 commits October 11, 2019 15:44

tests/xtimer_mutex_lock_timeout/main.c: remove unnecessary/wrong include

6183f50

tests/xtimer_mutex_lock_timeout/Makefile: adding ps for better testing

1fe8d33

tests/xtimer_mutex_lock_timeout/main.c: print empty line

f0da012

printing emty line after each test function to make the test easier to read

tests/xtimer_mutex_lock_timeout/main.c: Function to terminate

d9aec38

The function will terminate the thread and send the message m to target_pid.

tests/xtimer_mutex_lock_timeout/main.c: shell command description

15349ea

"(no-spin timeout)" instead of "long timeout" to make it consistent

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/new_test branch from 8e6bb25 to 15349ea Compare October 11, 2019 13:48

MichelRottleuthner approved these changes Oct 11, 2019

View reviewed changes

MichelRottleuthner merged commit cce1438 into RIOT-OS:master Oct 11, 2019

JulianHolzwarth deleted the pr/xtimer_mutex_lock_timeout/new_test branch October 11, 2019 15:43

JulianHolzwarth mentioned this pull request Oct 11, 2019

sys/xtimer/xtimer.c: _mutex_timeout() bug fix #11992

Merged

Conversation

JulianHolzwarth commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

MichelRottleuthner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JulianHolzwarth commented Aug 23, 2019

Uh oh!

JulianHolzwarth commented Aug 23, 2019

Uh oh!

JulianHolzwarth commented Aug 28, 2019

Uh oh!

JulianHolzwarth commented Sep 25, 2019

Uh oh!

MichelRottleuthner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JulianHolzwarth commented Sep 25, 2019

Uh oh!

MichelRottleuthner commented Sep 27, 2019

Uh oh!

JulianHolzwarth commented Sep 27, 2019

Uh oh!

JulianHolzwarth commented Sep 27, 2019

Uh oh!

JulianHolzwarth commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichelRottleuthner commented Oct 1, 2019

Uh oh!

JulianHolzwarth commented Oct 2, 2019

Uh oh!

JulianHolzwarth commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichelRottleuthner commented Oct 7, 2019

Uh oh!

JulianHolzwarth commented Oct 11, 2019

JulianHolzwarth commented Aug 14, 2019 •

edited

Loading

JulianHolzwarth commented Sep 27, 2019 •

edited

Loading

JulianHolzwarth commented Oct 2, 2019 •

edited

Loading