xtimer/xtimer.c: xtimer_rmutex_lock_timeout

[JulianHolzwarth]

Contribution description

This pr adds a new function xtimer_rmutex_lock_timeout. The function tries to acquire a rmutex within a timeout and returns if it was successful. The function uses xtimer_mutex_lock_timeout.

Testing procedure

BOARD=native make -C tests/xtimer_rmutex_lock_timeout/ flash test
it outputs before #11660:

...
> rmutex_timeout_long_unlocked
rmutex_timeout_long_unlocked
starting test: xtimer rmutex lock timeout
OK

> rmutex_timeout_long_locked
rmutex_timeout_long_locked
starting test: xtimer rmutex lock timeout
OK

> rmutex_timeout_long_lower_unlock_locked
rmutex_timeout_long_lower_unlock_locked
starting test: xtimer rmutex lock timeout
MAIN THREAD: created thread
THREAD low prio: locked and waiting
MAIN THREAD: locking
THREAD low prio: unlocking
OK
MAIN THREAD: waiting for created thread to end
THREAD low prio: exiting

> rmutex_timeout_short_locked
rmutex_timeout_short_locked
starting test: xtimer rmutex lock timeout with short timeout and locked rmutex
...
Unexpected end of file in expect script at "child.expect("OK")" (tests/xtimer_rmutex_lock_timeout/tests/01-run.py:36)

after #11660 output:

...
> rmutex_timeout_long_unlocked
rmutex_timeout_long_unlocked
starting test: xtimer rmutex lock timeout
OK

> rmutex_timeout_long_locked
rmutex_timeout_long_locked
starting test: xtimer rmutex lock timeout
OK

> rmutex_timeout_long_lower_unlock_locked
rmutex_timeout_long_lower_unlock_locked
starting test: xtimer rmutex lock timeout
MAIN THREAD: created thread
THREAD low prio: locked and waiting
MAIN THREAD: locking
THREAD low prio: unlocking
OK
MAIN THREAD: waiting for created thread to end
THREAD low prio: exiting

> rmutex_timeout_short_locked
rmutex_timeout_short_locked
starting test: xtimer rmutex lock timeout with short timeout and locked rmutex
OK

> rmutex_timeout_short_unlocked
rmutex_timeout_short_unlocked
starting test: xtimer rmutex lock timeout with short timeout and unlocked rmutex
OK

> 

Issues/PRs references

depends on #11660

[JulianHolzwarth]

@MichelRottleuthner Can you look at this?

[vincent-d]

The implementation of the feature looks good, it's how I did in #13706 (I missed this one) and it works as expected.

I guess someone more used to writing RIOT tests might want to take another look at the test. Looks good to me.

[miri64]

I had a quick look at the tests and they do look fine to me. However, Murdock is not happy appearently :(

[MichelRottleuthner]

This PR seems to suffer from some C/C++ atomics clash. @maribu I saw that you worked on C++ wrapper related things - do you have an Idea what's going on there?

[maribu]

This PR seems to suffer from some C/C++ atomics clash. @maribu I saw that you worked on C++ wrapper related things - do you have an Idea what's going on there?

C11 atomics and C++ atomics are incompatible. There is no way of accessing C++ atomics variables from C and C11 atomics form C++. Access to C11 atomics from C++ would only be possible by writing C functions (no header-only static inline functions wrapped in extern "C" { }, but real external c functions) and use these for accessing the variable.

Luckily, there is currently no use case where C++ code needs to access C11 atomics in RIOT, except for knowing the size and alignment requirements of the C11 atomics for allocation. E.g. in order to allocate an rmutex_t (e.g. by declaring a rmutex_t mut variable on the stack) from C++, the C++ compiler needs to know the size of rmutex_t and thus the size of its C11 atomic members.

c11_atomics_compat.hpp provides a type of the same size, alignment, and name as the C11 atomics under C++. (Those types are all structs with a single member called do_not_access_from_cpp, so that C++ developers trying to access these types are hopefully aware that they do nasty stuff.)

This PR uses the wrapper only for declaring structs containing C11 atomics to allow C++ code to allocate those types. So it is used correctly here :-)

[MichelRottleuthner]

Thanks a lot for the detailed information @maribu. Do you see any reason against adding the following to sys/include/c11_atomics_compat.hpp to work around the fact that esp32 vendor code defines it's own version of ATOMIC_VAR_INIT?

diff --git a/sys/include/c11_atomics_compat.hpp b/sys/include/c11_atomics_compat.hpp
index fbb6ef1aa..dff371fb7 100644
--- a/sys/include/c11_atomics_compat.hpp
+++ b/sys/include/c11_atomics_compat.hpp
@@ -36,7 +36,9 @@
  * atomic_int foo = ATOMIC_VAR_INIT(42);
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
+#ifndef ATOMIC_VAR_INIT
 #define ATOMIC_VAR_INIT(x) { x }
+#endif
 
 /**
   * @brief Type with the same alignment and size as `atomic_bool`

[maribu]

Do you see any reason against adding the following to sys/include/c11_atomics_compat.hpp to work around the fact that esp32 vendor code defines it's own version of ATOMIC_VAR_INIT?

tl;dr: I'd say it's fine.

In detal: It is a pity that both C11 atomics and C++ atomics have the very same macro to initialize their atomic variables. If both C11 atomics and C++ atomics are included and the define is actually different (which is not the case for the ESP toolchains), this would cause issues.

However, this macro is only needed for legacy toolchains that lack full C11 atomic / C++11 atomic support. (With full C11 atomic support you don't need to do atomic_int = ATOMIC_VAR_INIT(42), but you can just go with atomic_int = 42. Recent version of C++ also don't require its use and have even deprecated it.) So once the MSP430 toolchain is updated we can just drop the define and clean up some code using it. (And we also need to update the AVR toolchain in the docker image. It is a pity that - unlike all other major and many minor distros - Ubuntu doesn't maintain their AVR packages.)

[MichelRottleuthner]

I ran the test on native, esp32, pba-d-01-kw2x, and nucleo-f103rb.
While it works on native, it seems to fail on all real boards because of the following output hiccup:

> rmutex_timeout_long_locked_low
 rmutex_timeout_long_locked_low
starting test: xtimer rmutex lock timeout
THREAD low prio: start
MAIN THREAD: calling xtimer_rmutex_lock_timeout
THREAD low error: rmutex timed out
MAIN THREAD: waiting for created thread to end
p waiting for crTHREAD low prio: exiting low

> Timeout in expect script at "child.expect("OK")" (tests/xtimer_rmutex_lock_timeout/tests/01-run.py:32)

I also think it would be worth to strip away any output that is not absolutely necessary. That way we should get rid of (at least some) BOARD_INSUFFICIENT_MEMORY entries.
For automatic tests, covering more boards should IMO have much higher priority than "a bit more self-explaining shell interface".
E.g. the test command "rmutex_timeout_long_locked_low" and its description "lock low-prio-locked-rmutex from high-prio-thread (no-spin timeout)" is (and can never be) precise on describing the tested case in detail. Instead something like "t1" and "see main.c" would be a lot shorter and more explicit. When failing, everyone will look at the the code anyway.
People who run the test manually and want to know what the test is doing in detail also wouldn't get this from the command description currently.
Proposal: only keep outputs needed for test execution and result interpretation of the test script and move detailed descriptions to code comments.

[Edit]:
please also adapt the commit messages of sys/include/vfs.h: fix mulle atomic problem and sys/include/c11_atomics_compat.hpp: fix murdock problem to something more explicit ;)

[JulianHolzwarth]

@MichelRottleuthner reduced the output. Also what should the commit message say?
Fixes a problem with redefining ATOMIC_VAR_INIT for esp32
and the other one:
use c11_atomics_compat.hpp header wrapper
to solve a problem with c and c++ atomic

[JulianHolzwarth]

Also can i squash?

[JulianHolzwarth]

reduced size

[MichelRottleuthner]

Thanks for reducing the size. It should fit all platforms now. Though, you forgot to update the BOARD_INSUFFICIENT_MEMORY list accordingly. And you also didn't change the commit messages as we agreed on before ;)
Feel free to squash that in directly and ping me again then.

[JulianHolzwarth]

@MichelRottleuthner ping

[MichelRottleuthner]

I ran the provided test again on native, esp32, pba-d-01-kw2x, and nucleo-f103rb: it works as expected now.
All requests were addressed -> ACK