tests/xtimer_mutex_lock_timeout: add simple case test by JulianHolzwarth · Pull Request #11679 · RIOT-OS/RIOT

JulianHolzwarth · 2019-06-12T12:03:18Z

Contribution description

This pr implements normal test case tests for xtimer_mutex_lock_timeout in xtimer.c . As the timeout 1000us or 1 millisecond is used to make sure it does not spin. And adds a short comment for the struct it uses (mutex_thread_t). Also because the integer timeout in the struct is modified from interupt context it must be volatile.

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

comment for mutex_thread_t

timeout is modified from interupt context so must be volatile

cladmi · 2019-06-12T14:15:38Z

The test successfully ran on arduino-mega2560 frdm-k64f frdm-kw41z msba2 mulle nrf52dk nucleo-f103rb pba-d-01-kw2x sltb001a stm32f3discovery and locally on native iotlab-m3 samr21-xpro.

cladmi

I agree with the functional part but still have some remarks with documentation/naming. Please see inline.

tests/xtimer_mutex_lock_timeout/main.c

tests/xtimer_mutex_lock_timeout/tests/01-run.py

cladmi · 2019-06-12T14:23:39Z

tests/xtimer_mutex_lock_timeout/main.c

+ * @brief   shell command to test xtimer_mutex_lock_timeout
+ *
+ * the mutex is locked before the function call and
+ * the timeout is greater than XTIMER_BACKOFF (no spinning)


Please update in the direction, "the timeout will occur while waiting on the mutex" it would explain more the case without the need to mention spinning.

cladmi · 2019-06-12T14:25:20Z

tests/xtimer_mutex_lock_timeout/main.c

+ * @brief   shell command to test xtimer_mutex_lock_timeout
+ *
+ * the mutex is not locked before the function call and
+ * the timeout is greater than XTIMER_BACKOFF (no spinning)


Please update in the direction, "the timeout callback will be removed before triggering" it would explain more the case.

cladmi · 2019-06-12T14:31:08Z

tests/xtimer_mutex_lock_timeout/main.c

+/**
+ * Foward declarations
+ */
+static int cmd_test_xtimer_mutex_lock_timeout_greater_backoff(int argc,


Maybe update the function names to not mention 'backoff' but more what is tested, so testing with a free mutex and long timeout. Would match the documentation updates.

Same for the other function.

cladmi · 2019-06-12T14:31:24Z

sys/xtimer/xtimer.c

    mutex_t *mutex;
    thread_t *thread;
-    int timeout;
+    volatile int timeout;


Agreed with the change, the value is modified in another context.

cladmi · 2019-06-12T14:33:28Z

tests/xtimer_mutex_lock_timeout/main.c

+    mutex_t test_mutex = MUTEX_INIT;
+
+    /* timeout at one millisecond (1000 us) to make sure it does not spin. */
+    if (xtimer_mutex_lock_timeout(&test_mutex, 1000) == 0) {


Please put the 1000 as a macro with the comment before so it can be re-used in both functions.

JulianHolzwarth · 2019-06-12T15:16:50Z

I changed the documentation/naming. Do you have more suggestions and are you ok with my changes? @cladmi

cladmi

Some minor fixups inside. You can squash them directly.

Please squash everything, I will do a final round of check after.

tests/xtimer_mutex_lock_timeout/main.c

cladmi · 2019-06-12T15:51:47Z

tests/xtimer_mutex_lock_timeout/tests/01-run.py

+        child.sendline("help")
+        if child.expect_exact(["> ", pexpect.TIMEOUT], timeout=1) == 0:
+            break
+    child.sendline("mutex_timeout_n_spin_unlocked")


The tests commands must also be updated.

cladmi · 2019-06-13T09:05:29Z

Was notified IRL. Thanks for inline updating the documentation, I did not notice it was the documentation for the other function. https://github.com/RIOT-OS/RIOT/compare/5dc42d4f21306349841c1542d2471da685f3709a..50e9ac4d547ab8749f6b53f93f2c7baa89521ae4

cladmi · 2019-06-13T09:44:37Z

I re-read with more details and currently the tests do not verify that the mutex was indeed taken or not.

JulianHolzwarth · 2019-06-14T13:22:28Z

I re-read with more details and currently the tests do not verify that the mutex was indeed taken or not.

The new commit checks it.

JulianHolzwarth · 2019-06-14T13:31:04Z

Should be checking it (if mutex locked) now correctly.
@cladmi

cladmi · 2019-06-26T13:18:29Z

I get the expected output for the test, on samr21-xpro

Type '/exit' to exit.                                                                                                                                                                                                                                                                     
2019-06-26 15:13:41,145 - INFO # main(): This is RIOT! (Version: buildtest)                                                                                                                                                                                                               
2019-06-26 15:13:41,147 - INFO # Starting shell...                                                                                                                                                                                                                                        
> mutex_timeout_long_unlocked                                                                                                                                                                                                                                                             
2019-06-26 15:13:41,188 - INFO #  help                                                                                                                                                                                                                                                    
2019-06-26 15:13:41,191 - INFO # Command              Description                                                                                                                                                                                                                         
2019-06-26 15:13:41,195 - INFO # ---------------------------------------                                                                                                                                                                                                                  
2019-06-26 15:13:41,200 - INFO # mutex_timeout_long_unlocked unlocked mutex with long timeout                                                                                                                                                                                             
2019-06-26 15:13:41,205 - INFO # mutex_timeout_long_locked locked mutex with long timeout                                                                                                                                                                                                 
> 2019-06-26 15:13:41,242 - INFO #  mutex_timeout_long_unlocked                                                                                                                                                                                                                           
2019-06-26 15:13:41,246 - INFO # starting test: xtimer mutex lock timeout                                                                                                                                                                                                                 
2019-06-26 15:13:41,247 - INFO # OK                                                                                                                                                                                                                                                       
> mutex_timeout_long_locked                                                                                                                                                                                                                                                               
2019-06-26 15:13:41,302 - INFO #  mutex_timeout_long_locked                                                                                                                                                                                                                               
2019-06-26 15:13:41,306 - INFO # starting test: xtimer mutex lock timeout                                                                                                                                                                                                                 
2019-06-26 15:13:41,307 - INFO # OK

and the test successfully ran on arduino-mega2560 cc2650-launchpad frdm-k64f frdm-kw41z msba2 mulle nrf52dk nucleo-f103rb pba-d-01-kw2x sltb001a stm32f3discovery and locally on native iotlab-m3 samr21-xpro.

Please squash.

Adding a first normal test case where the mutex is unlocked and the timeout is long. The timer will not trigger in this test and instead wil be removed after getting the mutex.

New test function cmd_test_xtimer_mutex_lock_timeout_long_locked. In this test the mutex is locked and the timeout is long. When it works the thread continues running and stops waiting for the mutex and the function will return that it did not get the mutex.

JulianHolzwarth · 2019-06-26T13:40:15Z

@cladmi I squashed.

cladmi

ACK, reviewed and tested on many boards.

Note for reference: this currently only test the simple cases where the mutex is not changed while waiting, and the timing is not analyzed.

cladmi · 2019-06-26T14:28:35Z

First pull request merged! Well done.

JulianHolzwarth · 2019-06-26T14:32:10Z

Thank you.

JulianHolzwarth added 2 commits June 5, 2019 14:16

xtimer/xtimer.c document mutex_thread_t

301d88c

comment for mutex_thread_t

xtimer/xtimer.c: fix timeout is volatile

1fff551

timeout is modified from interupt context so must be volatile

JulianHolzwarth mentioned this pull request Jun 12, 2019

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

Merged

6 tasks

JulianHolzwarth changed the title ~~Pr/xtimer mutex lock timeout/first tests~~ tests/xtimer_mutex_lock_timeout Jun 12, 2019

cladmi added this to the Release 2019.07 milestone Jun 12, 2019

JulianHolzwarth changed the title ~~tests/xtimer_mutex_lock_timeout~~ tests/xtimer_mutex_lock_timeout add simple case test Jun 12, 2019

JulianHolzwarth changed the title ~~tests/xtimer_mutex_lock_timeout add simple case test~~ tests/xtimer_mutex_lock_timeout: add simple case test Jun 12, 2019

cladmi suggested changes Jun 12, 2019

View reviewed changes

cladmi requested review from MichelRottleuthner and MrKevinWeiss June 12, 2019 14:34

cladmi reviewed Jun 12, 2019

View reviewed changes

tests/xtimer_mutex_lock_timeout/main.c Outdated Show resolved Hide resolved

tests/xtimer_mutex_lock_timeout/main.c Outdated Show resolved Hide resolved

tests/xtimer_mutex_lock_timeout/main.c Outdated Show resolved Hide resolved

cladmi reviewed Jun 12, 2019

View reviewed changes

cladmi removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 12, 2019

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/first_tests branch from 5dc42d4 to 50e9ac4 Compare June 12, 2019 16:34

cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 13, 2019

cladmi removed the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jun 13, 2019

JulianHolzwarth added 2 commits June 26, 2019 15:26

tests/xtimer_mutex_lock_timeout: minimal xtimer_mutex_lock_timeout test

f207c6a

Adding a first normal test case where the mutex is unlocked and the timeout is long. The timer will not trigger in this test and instead wil be removed after getting the mutex.

JulianHolzwarth force-pushed the pr/xtimer_mutex_lock_timeout/first_tests branch from 99078f4 to b3d2324 Compare June 26, 2019 13:35

cladmi approved these changes Jun 26, 2019

View reviewed changes

cladmi merged commit eab0a88 into RIOT-OS:master Jun 26, 2019

JulianHolzwarth deleted the pr/xtimer_mutex_lock_timeout/first_tests branch June 26, 2019 14:42

Conversation

JulianHolzwarth commented Jun 12, 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!

cladmi commented Jun 12, 2019

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cladmi Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

JulianHolzwarth commented Jun 12, 2019

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cladmi Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

JulianHolzwarth Jun 12, 2019

Choose a reason for hiding this comment

Uh oh!

cladmi commented Jun 13, 2019

Uh oh!

cladmi commented Jun 13, 2019

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

cladmi commented Jun 26, 2019

Uh oh!

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

Uh oh!

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Jun 26, 2019

Uh oh!

JulianHolzwarth commented Jun 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JulianHolzwarth commented Jun 12, 2019 •

edited

Loading

JulianHolzwarth commented Jun 14, 2019 •

edited

Loading

JulianHolzwarth commented Jun 14, 2019 •

edited

Loading

JulianHolzwarth commented Jun 26, 2019 •

edited

Loading