diff --git a/Documentation/epicardium/mutex.rst b/Documentation/epicardium/mutex.rst new file mode 100644 index 0000000000000000000000000000000000000000..e81df47c6bf1e2c036d4a56aedae5ed30d364c8f --- /dev/null +++ b/Documentation/epicardium/mutex.rst @@ -0,0 +1,94 @@ +Mutex +===== +Ontop of FreeRTOS, we have our own mutex implementation. **Never use the +FreeRTOS mutexes directly! Always use this abstraction layer instead**. This +mutex implementation tries to make reasoning about program flow and locking +behavior easier. And most importantly tries to help with debugging possible +dead-locks. + +Design +------ +There are a few guiding design principles: + +- Mutexes can only be used from tasks, **never** from interrupts! +- Timers can use mutexes, but only with :c:func:`mutex_trylock`, **never** with + :c:func:`mutex_lock` (Because they are not allowed to block). +- Locking can *never* fail (if it does, we consider this a fatal error ⇒ panic). +- No recursive locking. +- An unlock can only occur from the task which previously acquired the mutex. +- An unlock is only allowed if the mutex was previously acquired. + +For a more elaborate explanation of the rationale behind these rules take a +look at the :ref:`mutex-design-reasons`. + +Definitions +----------- +.. c:autodoc:: epicardium/modules/mutex.h + +.. _mutex-design-reasons: + +Reasons for this Design +----------------------- + +Only from tasks +^^^^^^^^^^^^^^^ +Cases where it might make sense to acquire a lock from an IRQ are very hard to +get right. They often involve the lock and unlock being in different functions +and thus the correctness of the code is resting on the assumption that those +functions are called in the correct order, also in respect to the rest of the +related code. By disallowing this entirely, we force code-flow to be more +linear and hopefully easier to reason about. + +An exception is with software timers: While they technically *can* acquire a +mutex (using :c:func:`mutex_trylock`), I strongly recommend **not** doing that. +Because :c:func:`mutex_trylock` can fail, doing these kind of timers correctly +is not trivial. Instead, use a task and block it using :c:func:`mutex_lock`. + +Locking can *never* fail +^^^^^^^^^^^^^^^^^^^^^^^^ +From a technical standpoint this might seem unintuitive. But when looking at +it from the semantics of what it means for a lock to fail, it makes total +sense: There are two cases which could be considered failure of a lock: + +1. **A dead-lock condition**. Dead-locks should not be catched by code, but by + review of the code earlier on. Tools like static- or runtime-analysis are + the proper way to detect possible dead-locks. Going the windows-route of + trying to write the OS defensive against itself is not going to end well. + Fix the code which might dead-lock instead of slapping a band-aid ontop. +2. **Another task blocking the lock for a long time**. This is a bit more + tricky: FreeRTOS provides the ability to add a timeout to locking attempts + to catch these cases. But this is also a bad idea. Think about it: What + would you even do after the locking attempt timed out? You cannot access + the mutexed data/peripheral anyway because someone else is still modifying + it. Even if you were to kill the offending task, the data/peripheral might + be in a totally unpredictable state. It is also very hard to define timeout + values that are relaxed enough for all possible code-paths acquiring the + lock. You would have to take situations like the task holding the mutex + being preempted by a long-running high priority task into account. So + instead, we provide :c:func:`mutex_trylock`: If the mutex cannot be + acquired immediately you have the chance to choose an alternative code-path + and try again later. + +No recursive locking +^^^^^^^^^^^^^^^^^^^^ +While recursive locking might seem to make a few cases easier to deal with, it +carries quite a few implications which make it not worthwhile. It makes +analysis of locking chains a *lot* harder and in some cases even impossible +(for proving 100% correcteness). We should strive for a design which is as +simple as possible and as obvious to understand as possible. Keeping locking +hierarchy flat helps quite a lot with that. + +Only unlock from the acquiring task +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +This one should be obvious but FreeRTOS does not enforce this ... By stating +that locking can never fail, we also won't ever have a situation where +force-unlocking a forgein lock might be necessary. + +Only unlock once after acquisition +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +As a pseude-recursive locking implementation, sometimes the +:c:func:`mutex_unlock` call is written in a way that allows unlocking an +already unlocked mutex. But this only encourages bad practices like unlocking +everything again at the end of a function "just to be sure". Code should be +written in a way where the lock/unlock pair is immediately recognizable as +belonging together. diff --git a/Documentation/index.rst b/Documentation/index.rst index 44ecfcd04f8fb330aea3ae070643895bc70d9511..2d51547a1def13ebe0239310f33e07809ac63836 100644 --- a/Documentation/index.rst +++ b/Documentation/index.rst @@ -52,6 +52,7 @@ Last but not least, if you want to start hacking the lower-level firmware, the debugger pycardium-guide memorymap + epicardium/mutex epicardium/sensor-streams .. toctree:: diff --git a/epicardium/FreeRTOSConfig.h b/epicardium/FreeRTOSConfig.h index b731f6a9abdee7556f073936d86aba649e72a8d1..22c6dfb0f050c38c2d33b7f1d64b1e7940d8ea97 100644 --- a/epicardium/FreeRTOSConfig.h +++ b/epicardium/FreeRTOSConfig.h @@ -52,6 +52,8 @@ #define INCLUDE_vTaskDelay 1 #define INCLUDE_uxTaskGetStackHighWaterMark 1 #define INCLUDE_xTimerPendFunctionCall 1 +#define INCLUDE_xSemaphoreGetMutexHolder 1 + /* Allow static allocation of data structures */ #define configSUPPORT_STATIC_ALLOCATION 1 diff --git a/epicardium/modules/meson.build b/epicardium/modules/meson.build index 022397ca26e3866ef9a72472a7621e4cd6dbfec2..8ead5e72c4cc80e3fe0601d713784028cc16645f 100644 --- a/epicardium/modules/meson.build +++ b/epicardium/modules/meson.build @@ -14,6 +14,7 @@ module_sources = files( 'light_sensor.c', 'log.c', 'max30001.c', + 'mutex.c', 'panic.c', 'personal_state.c', 'pmic.c', diff --git a/epicardium/modules/mutex.c b/epicardium/modules/mutex.c new file mode 100644 index 0000000000000000000000000000000000000000..9d58eec080977d93f69f275350a5101a15d4a64b --- /dev/null +++ b/epicardium/modules/mutex.c @@ -0,0 +1,55 @@ +#include "modules/mutex.h" + +#include <assert.h> + +void _mutex_create(struct mutex *m, const char *name) +{ + /* Assert that the mutex has not been initialized already */ + assert(m->name == NULL); + + /* + * The name is just the parameter stringified which is almost always a + * pointer. If it is, skip over the '&' because it adds no value as + * part of the name. + */ + if (name[0] == '&') { + m->name = &name[1]; + } else { + m->name = name; + } + + m->_rtos_mutex = xSemaphoreCreateMutexStatic(&m->_rtos_mutex_data); +} + +void mutex_lock(struct mutex *m) +{ + int ret = xSemaphoreTake(m->_rtos_mutex, portMAX_DELAY); + + /* Ensure locking was actually successful */ + assert(ret == pdTRUE); +} + +bool mutex_trylock(struct mutex *m) +{ + int ret = xSemaphoreTake(m->_rtos_mutex, 0); + return ret == pdTRUE; +} + +void mutex_unlock(struct mutex *m) +{ + /* Ensure that only the owner can unlock a mutex */ + assert(mutex_get_owner(m) == xTaskGetCurrentTaskHandle()); + + int ret = xSemaphoreGive(m->_rtos_mutex); + + /* + * Ensure that unlocking was successful; that is, the mutex must have + * been acquired previously (no multiple unlocks). + */ + assert(ret == pdTRUE); +} + +TaskHandle_t mutex_get_owner(struct mutex *m) +{ + return xSemaphoreGetMutexHolder(m->_rtos_mutex); +} diff --git a/epicardium/modules/mutex.h b/epicardium/modules/mutex.h new file mode 100644 index 0000000000000000000000000000000000000000..1c3055996ac2142bee634d9fd9f4c09b3233d69d --- /dev/null +++ b/epicardium/modules/mutex.h @@ -0,0 +1,82 @@ +#ifndef _MUTEX_H +#define _MUTEX_H + +#ifndef __SPHINX_DOC +/* Some headers are not recognized by hawkmoth for some odd reason */ +#include <stddef.h> +#include <stdbool.h> + +#include "FreeRTOS.h" +#include "semphr.h" +#else +typedef unsigned int size_t; +typedef _Bool bool; + +typedef void *TaskHandle_t; +typedef void *SemaphoreHandle_t; +typedef int StaticSemaphore_t; +#endif /* __SPHINX_DOC */ + + +/** + * Mutex data type. + */ +struct mutex { + /* Name of this mutex, kept for debugging purposes. */ + const char *name; + + /* FreeRTOS mutex data structures. */ + SemaphoreHandle_t _rtos_mutex; + StaticSemaphore_t _rtos_mutex_data; +}; + +/** + * Create a new mutex. + * + * Call this function as early as possible, in an obvious location so it is easy + * to find. Mutexes should be defined statically so they stay alive for the + * entire run-time of the firmware. + */ +#define mutex_create(mutex) _mutex_create(mutex, #mutex) +void _mutex_create(struct mutex *m, const char *name); + +/** + * Lock a mutex. + * + * If the mutex is held by another task, :c:func:`mutex_lock` will block the + * current task until the mutex is unlocked. + * + * .. warning:: + * + * This function is **not** safe to use in a timer! + */ +void mutex_lock(struct mutex *m); + +/** + * Try locking a mutex. + * + * If the mutex is currently locked by another task, :c:func:`mutex_trylock` + * will return ``false`` immediately. If the attmept to lock was successful, it + * will return ``true``. + * + * This funciton is safe for use in timers. + */ +bool mutex_trylock(struct mutex *m); + +/** + * Unlock a mutex. + * + * You **must** call this function from the same task which originally locked + * the mutex. + */ +void mutex_unlock(struct mutex *m); + +/** + * Get the current owner of the mutex. + * + * Returns the task-handle of the task currently holding the mutex. If the + * mutex is unlocked, ``NULL`` is returned. + */ +TaskHandle_t mutex_get_owner(struct mutex *m); + +#endif /* _MUTEX_H */