From 3b508c8b3787ada98a042699a1bc201dfc28d8bb Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Tue, 23 Aug 2016 17:10:18 +0800 Subject: [PATCH 01/10] esp32 syscalls.c: Use rom/uart.h for uart_tx_one_char prototype --- components/esp32/syscalls.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/components/esp32/syscalls.c b/components/esp32/syscalls.c index f6da4a52a7..5c0b870c4f 100644 --- a/components/esp32/syscalls.c +++ b/components/esp32/syscalls.c @@ -20,12 +20,11 @@ #include #include #include "rom/libc_stubs.h" +#include "rom/uart.h" #include "freertos/FreeRTOS.h" #include "freertos/portmacro.h" #include "freertos/task.h" -int uart_tx_one_char(uint8_t c); - void abort() { do { From 9921e60f559e87b243607e1972f9dbbfc26c84be Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Tue, 23 Aug 2016 17:10:41 +0800 Subject: [PATCH 02/10] freertos: Change variable name in comment --- components/freertos/port.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/freertos/port.c b/components/freertos/port.c index 8f0a617af1..05932c9146 100644 --- a/components/freertos/port.c +++ b/components/freertos/port.c @@ -249,7 +249,7 @@ void vPortStoreTaskMPUSettings( xMPU_SETTINGS *xMPUSettings, const struct xMEMOR /* * Wrapper for the Xtensa compare-and-set instruction. This subroutine will atomically compare - * *addr to compare, and if it's the same, will set *addr to set. It will return the old value + * *mux to compare, and if it's the same, will set *mux to set. It will return the old value * of *addr. * * Note: the NOPs are needed on the ESP31 processor but superfluous on the ESP32. From bd2f9e03f06544d0e0f3d9072e82d4cc15effbe6 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Tue, 23 Aug 2016 17:09:44 +0800 Subject: [PATCH 03/10] Add newlib libc locking using FreeRTOS primitives --- components/esp32/include/soc/cpu.h | 36 +++++ components/esp32/syscalls.c | 204 +++++++++++++++++++++++++++-- 2 files changed, 226 insertions(+), 14 deletions(-) create mode 100644 components/esp32/include/soc/cpu.h diff --git a/components/esp32/include/soc/cpu.h b/components/esp32/include/soc/cpu.h new file mode 100644 index 0000000000..fdcf62190e --- /dev/null +++ b/components/esp32/include/soc/cpu.h @@ -0,0 +1,36 @@ +// Copyright 2010-2016 Espressif Systems (Shanghai) PTE LTD +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at + +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef _SOC_CPU_H +#define _SOC_CPU_H + +#include "xtensa/corebits.h" + +/* C macros for xtensa special register read/write/exchange */ + +#define RSR(reg, curval) asm volatile ("rsr %0, " #reg : "=r" (curval)); +#define WSR(reg, newval) asm volatile ("wsr %0, " #reg : : "r" (newval)); +#define XSR(reg, swapval) asm volatile ("xsr %0, " #reg : "+r" (swapval)); + +/* Return true if the CPU is in an interrupt context + (PS.UM == 0) +*/ +static inline bool cpu_in_interrupt_context(void) +{ + uint32_t ps; + RSR(PS, ps); + return (ps & PS_UM) == 0; +} + +#endif diff --git a/components/esp32/syscalls.c b/components/esp32/syscalls.c index 5c0b870c4f..a087c2caaa 100644 --- a/components/esp32/syscalls.c +++ b/components/esp32/syscalls.c @@ -19,9 +19,12 @@ #include #include #include +#include "esp_attr.h" #include "rom/libc_stubs.h" #include "rom/uart.h" +#include "soc/cpu.h" #include "freertos/FreeRTOS.h" +#include "freertos/semphr.h" #include "freertos/portmacro.h" #include "freertos/task.h" @@ -157,37 +160,210 @@ ssize_t _read_r(struct _reent *r, int fd, void * dst, size_t size) { return 0; } -// TODO: implement locks via FreeRTOS mutexes -void _lock_init(_lock_t *lock) { +/* Notes on our newlib lock implementation: + * + * - lock_t is int. This value which is an index into a lock table entry, + * with a high bit flag for "lock initialised". + * - Lock table is a table of FreeRTOS mutexes. + * - Locks are no-ops until the FreeRTOS scheduler is running. + * - Writing to the lock table is protected by a spinlock. + * + */ + +/* Maybe make this configurable? + It's MAXFDs plus a few static locks. */ +#define LOCK_TABLE_SIZE 32 + +static xSemaphoreHandle lock_table[LOCK_TABLE_SIZE]; +static portMUX_TYPE lock_table_spinlock = portMUX_INITIALIZER_UNLOCKED; + +#define LOCK_INDEX_INITIALISED_FLAG (1<<31) + +/* Utility function to look up a particular lock in the lock table and + * return a pointer to its xSemaphoreHandle entry. */ +static inline IRAM_ATTR xSemaphoreHandle *get_lock_table_entry(_lock_t *lock) +{ + if (*lock & LOCK_INDEX_INITIALISED_FLAG) { + return &lock_table[*lock & ~LOCK_INDEX_INITIALISED_FLAG]; + } + return NULL; } -void _lock_init_recursive(_lock_t *lock) { +static inline IRAM_ATTR bool get_scheduler_started(void) +{ + int s = xTaskGetSchedulerState(); + return s != taskSCHEDULER_NOT_STARTED; } -void _lock_close(_lock_t *lock) { +/* Initialise the given lock by inserting a new mutex semaphore of + type mutex_type into the lock table. +*/ +static void IRAM_ATTR lock_init_generic(_lock_t *lock, uint8_t mutex_type) { + if (!get_scheduler_started()) { + return; /* nothing to do until the scheduler is running */ + } + + portENTER_CRITICAL(&lock_table_spinlock); + if (*lock & LOCK_INDEX_INITIALISED_FLAG) { + /* Lock already initialised (either we didn't check earlier, + or it got initialised while we were waiting for the + spinlock.) */ + configASSERT(*get_lock_table_entry(lock) != NULL); + } + else + { + /* Create a new semaphore and save it in the lock table. + + this is a bit of an API violation, as we're calling the + private function xQueueCreateMutex(x) directly instead of + the xSemaphoreCreateMutex / xSemaphoreCreateRecursiveMutex + wrapper functions... + + The better alternative would be to pass pointers to one of + the two xSemaphoreCreate___Mutex functions, but as FreeRTOS + implements these as macros instead of inline functions + (*party like it's 1998!*) it's not possible to do this + without writing wrappers. Doing it this way seems much less + spaghetti-like. + */ + xSemaphoreHandle new_sem = xQueueCreateMutex(mutex_type); + if (!new_sem) { + abort(); /* No more semaphores available or OOM */ + } + bool success = false; + for (int i = 0; i < LOCK_TABLE_SIZE && !success; i++) { + if (lock_table[i] == 0) { + lock_table[i] = new_sem; + *lock = i | LOCK_INDEX_INITIALISED_FLAG; + success = true; + } + } + if (!success) { + abort(); /* we have more locks than lock table entries */ + } + } + portEXIT_CRITICAL(&lock_table_spinlock); } -void _lock_close_recursive(_lock_t *lock) { +void IRAM_ATTR _lock_init(_lock_t *lock) { + lock_init_generic(lock, queueQUEUE_TYPE_MUTEX); } -void _lock_acquire(_lock_t *lock) { +void IRAM_ATTR _lock_init_recursive(_lock_t *lock) { + lock_init_generic(lock, queueQUEUE_TYPE_RECURSIVE_MUTEX); } -void _lock_acquire_recursive(_lock_t *lock) { +/* Free the mutex semaphore pointed to by *lock, and zero out + the entry in the lock table. + + Note that FreeRTOS doesn't account for deleting mutexes while they + are held, and neither do we... so take care not to delete newlib + locks while they may be held by other tasks! +*/ +void IRAM_ATTR _lock_close(_lock_t *lock) { + if (*lock & LOCK_INDEX_INITIALISED_FLAG) { + portENTER_CRITICAL(&lock_table_spinlock); + xSemaphoreHandle *h = get_lock_table_entry(lock); +#if (INCLUDE_xSemaphoreGetMutexHolder == 1) + configASSERT(xSemaphoreGetMutexHolder(*h) != NULL); /* mutex should not be held */ +#endif + vSemaphoreDelete(*h); + *h = NULL; + *lock = 0; + portEXIT_CRITICAL(&lock_table_spinlock); + } } -int _lock_try_acquire(_lock_t *lock) { - return 0; +/* Acquire the mutex semaphore indexed by lock, wait up to delay ticks. + mutex_type is queueQUEUE_TYPE_RECURSIVE_MUTEX or queueQUEUE_TYPE_MUTEX +*/ +static int IRAM_ATTR lock_acquire_generic(_lock_t *lock, uint32_t delay, uint8_t mutex_type) { + xSemaphoreHandle *h = get_lock_table_entry(lock); + if (!h) { + if (!get_scheduler_started()) { + return 0; /* locking is a no-op before scheduler is up, so this "succeeds" */ + } + /* lazy initialise lock - might have had a static initializer in newlib (that we don't use), + or _lock_init might have been called before the scheduler was running... */ + lock_init_generic(lock, mutex_type); + } + h = get_lock_table_entry(lock); + configASSERT(h != NULL); + + BaseType_t success; + if (cpu_in_interrupt_context()) { + /* In ISR Context */ + if (mutex_type == queueQUEUE_TYPE_RECURSIVE_MUTEX) { + abort(); /* recursive mutexes make no sense in ISR context */ + } + BaseType_t higher_task_woken = false; + success = xSemaphoreTakeFromISR(*h, &higher_task_woken); + if (!success && delay > 0) { + abort(); /* Tried to block on mutex from ISR, couldn't... rewrite your program to avoid libc interactions in ISRs! */ + } + /* TODO: deal with higher_task_woken */ + } + else { + /* In task context */ + if (mutex_type == queueQUEUE_TYPE_RECURSIVE_MUTEX) { + success = xSemaphoreTakeRecursive(*h, delay); + } else { + success = xSemaphoreTake(*h, delay); + } + } + + return (success == pdTRUE) ? 0 : -1; } -int _lock_try_acquire_recursive(_lock_t *lock) { - return 0; +void IRAM_ATTR _lock_acquire(_lock_t *lock) { + lock_acquire_generic(lock, portMAX_DELAY, queueQUEUE_TYPE_MUTEX); } -void _lock_release(_lock_t *lock) { +void IRAM_ATTR _lock_acquire_recursive(_lock_t *lock) { + lock_acquire_generic(lock, portMAX_DELAY, queueQUEUE_TYPE_RECURSIVE_MUTEX); } -void _lock_release_recursive(_lock_t *lock) { +int IRAM_ATTR _lock_try_acquire(_lock_t *lock) { + return lock_acquire_generic(lock, 0, queueQUEUE_TYPE_MUTEX); +} + +int IRAM_ATTR _lock_try_acquire_recursive(_lock_t *lock) { + return lock_acquire_generic(lock, 0, queueQUEUE_TYPE_RECURSIVE_MUTEX); +} + +/* Release the mutex semaphore indexed by lock. + mutex_type is queueQUEUE_TYPE_RECURSIVE_MUTEX or queueQUEUE_TYPE_MUTEX +*/ +static void IRAM_ATTR lock_release_generic(_lock_t *lock, uint8_t mutex_type) { + xSemaphoreHandle *h = get_lock_table_entry(lock); + if (h == NULL) { + /* This is probably because the scheduler isn't running yet, + or the scheduler just started running and some code was + "holding" a not-yet-initialised lock... */ + return; + } + + if (cpu_in_interrupt_context()) { + if (mutex_type == queueQUEUE_TYPE_RECURSIVE_MUTEX) { + abort(); /* indicates logic bug, it shouldn't be possible to lock recursively in ISR */ + } + BaseType_t higher_task_woken = false; + xSemaphoreGiveFromISR(*h, &higher_task_woken); + } else { + if (mutex_type == queueQUEUE_TYPE_RECURSIVE_MUTEX) { + xSemaphoreGiveRecursive(*h); + } else { + xSemaphoreGive(*h); + } + } +} + +void IRAM_ATTR _lock_release(_lock_t *lock) { + lock_release_generic(lock, queueQUEUE_TYPE_MUTEX); +} + +void IRAM_ATTR _lock_release_recursive(_lock_t *lock) { + lock_release_generic(lock, queueQUEUE_TYPE_RECURSIVE_MUTEX); } static struct _reent s_reent; @@ -238,7 +414,7 @@ static struct syscall_stub_table s_stub_table = { ._lock_init = &_lock_init, ._lock_init_recursive = &_lock_init_recursive, ._lock_close = &_lock_close, - ._lock_close_recursive = &_lock_close_recursive, + ._lock_close_recursive = &_lock_close, ._lock_acquire = &_lock_acquire, ._lock_acquire_recursive = &_lock_acquire_recursive, ._lock_try_acquire = &_lock_try_acquire, From 94104f0fe87f97dd3289110d087eee54154146aa Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Tue, 23 Aug 2016 17:10:52 +0800 Subject: [PATCH 04/10] Add hack of explicitly locking stdout This shouldn't be necessary as stdout is already locked by libc (see comment.) Not sure which part isn't working. --- components/esp32/syscalls.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/components/esp32/syscalls.c b/components/esp32/syscalls.c index a087c2caaa..8978154240 100644 --- a/components/esp32/syscalls.c +++ b/components/esp32/syscalls.c @@ -138,6 +138,13 @@ int _open_r(struct _reent *r, const char * path, int flags, int mode) { ssize_t _write_r(struct _reent *r, int fd, const void * data, size_t size) { const char* p = (const char*) data; if (fd == STDOUT_FILENO) { + /* THIS IS A HACK!!! The stdout "file" should be locked while + this code is called (it's locked fflush.c:280 before + __sflush_r is called.) It shouldn't be necessary to + re-lock, but due to some unknown bug it is... + */ + static _lock_t stdout_lock; + _lock_acquire_recursive(&stdout_lock); while(size--) { #if CONFIG_NEWLIB_STDOUT_ADDCR if (*p=='\n') { @@ -147,6 +154,7 @@ ssize_t _write_r(struct _reent *r, int fd, const void * data, size_t size) { uart_tx_one_char(*p); ++p; } + _lock_release_recursive(&stdout_lock); } return size; } From 93c92f7a5bfa2a32355eb048152bacc5d467d689 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 24 Aug 2016 16:01:41 +0800 Subject: [PATCH 05/10] FreeRTOS: Configure configASSERT fail behaviour, abort() by default --- components/freertos/Kconfig | 25 +++++++++++++++++++ .../include/freertos/FreeRTOSConfig.h | 19 +++++++++++--- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/components/freertos/Kconfig b/components/freertos/Kconfig index 89227a7ddf..037e795bc9 100644 --- a/components/freertos/Kconfig +++ b/components/freertos/Kconfig @@ -121,5 +121,30 @@ config FREERTOS_DEBUG_OCDAWARE The FreeRTOS panic and unhandled exception handers can detect a JTAG OCD debugger and instead of panicking, have the debugger stop on the offending instruction. +choice FREERTOS_ASSERT + prompt "FreeRTOS assertions" + default FREERTOS_ASSERT_FAIL_ABORT + help + Failed FreeRTOS configASSERT() assertions can be configured to + behave in different ways. + +config FREERTOS_ASSERT_FAIL_ABORT + bool "abort() on failed assertions" + help + If a FreeRTOS configASSERT() fails, FreeRTOS will abort() and + halt execution. The panic handler can be configured to handle + the outcome of an abort() in different ways. + +config FREERTOS_ASSERT_FAIL_PRINT_CONTINUE + bool "Print and continue failed assertions" + help + If a FreeRTOS assertion fails, print it out and continue. + +config FREERTOS_ASSERT_DISABLE + bool "Disable FreeRTOS assertions" + help + FreeRTOS configASSERT() will not be compiled into the binary. + +endchoice endmenu diff --git a/components/freertos/include/freertos/FreeRTOSConfig.h b/components/freertos/include/freertos/FreeRTOSConfig.h index c0a86efed3..46abb9a3b8 100644 --- a/components/freertos/include/freertos/FreeRTOSConfig.h +++ b/components/freertos/include/freertos/FreeRTOSConfig.h @@ -106,12 +106,25 @@ #include "xtensa_config.h" -#if 1 +/* configASSERT behaviour */ #ifndef __ASSEMBLER__ #include "rom/ets_sys.h" -#define configASSERT(a) if (!(a)) ets_printf("%s:%d (%s)- assert failed!\n", __FILE__, __LINE__, __FUNCTION__) -#endif + +#if defined(CONFIG_FREERTOS_ASSERT_DISABLE) +#define configASSERT(a) /* assertions disabled */ +#elif defined(CONFIG_FREERTOS_ASSERT_FAIL_PRINT_CONTINUE) +#define configASSERT(a) if (!(a)) { \ + ets_printf("%s:%d (%s)- assert failed!\n", __FILE__, __LINE__, \ + __FUNCTION__); \ + } +#else /* CONFIG_FREERTOS_ASSERT_FAIL_ABORT */ +#define configASSERT(a) if (!(a)) { \ + ets_printf("%s:%d (%s)- assert failed!\n", __FILE__, __LINE__, \ + __FUNCTION__); \ + abort(); \ + } #endif +#endif /* def __ASSEMBLER__ */ /*----------------------------------------------------------- From 4b281af0f73297ef675f0aba0b1d3f3f13ffa623 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 24 Aug 2016 18:09:51 +0800 Subject: [PATCH 06/10] newlib locking: Turns out the "hack" is the way to make stdout thread-safe in newlib --- components/esp32/syscalls.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/components/esp32/syscalls.c b/components/esp32/syscalls.c index 8978154240..92bc33374a 100644 --- a/components/esp32/syscalls.c +++ b/components/esp32/syscalls.c @@ -138,12 +138,20 @@ int _open_r(struct _reent *r, const char * path, int flags, int mode) { ssize_t _write_r(struct _reent *r, int fd, const void * data, size_t size) { const char* p = (const char*) data; if (fd == STDOUT_FILENO) { - /* THIS IS A HACK!!! The stdout "file" should be locked while - this code is called (it's locked fflush.c:280 before - __sflush_r is called.) It shouldn't be necessary to - re-lock, but due to some unknown bug it is... + static _lock_t stdout_lock; /* lazily initialised */ + /* Even though newlib does stream locking on stdout, we need + a dedicated stdout UART lock... + + This is because each task has its own _reent structure with + unique FILEs for stdin/stdout/stderr, so these are + per-thread (lazily initialised by __sinit the first time a + stdio function is used, see findfp.c:235. + + It seems like overkill to allocate a FILE-per-task and lock + a thread-local stream, but I see no easy way to fix this + (pre-__sinit_, tasks have "fake" FILEs ie __sf_fake_stdout + which aren't fully valid.) */ - static _lock_t stdout_lock; _lock_acquire_recursive(&stdout_lock); while(size--) { #if CONFIG_NEWLIB_STDOUT_ADDCR From f5715ac28d12735c4505c6188d25e5ee7e9b6120 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 24 Aug 2016 18:10:52 +0800 Subject: [PATCH 07/10] FreeRTOS: Add xQueueGetMutexHolder support Enables it as a config option, but there's no overhead at all if the function is not called anywhere. --- components/freertos/include/freertos/FreeRTOSConfig.h | 2 ++ components/freertos/queue.c | 7 ++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/components/freertos/include/freertos/FreeRTOSConfig.h b/components/freertos/include/freertos/FreeRTOSConfig.h index 46abb9a3b8..e95abaa9cc 100644 --- a/components/freertos/include/freertos/FreeRTOSConfig.h +++ b/components/freertos/include/freertos/FreeRTOSConfig.h @@ -222,6 +222,8 @@ #define INCLUDE_vTaskDelay 1 #define INCLUDE_uxTaskGetStackHighWaterMark 1 +#define INCLUDE_xSemaphoreGetMutexHolder 1 + /* The priority at which the tick interrupt runs. This should probably be kept at 1. */ #define configKERNEL_INTERRUPT_PRIORITY 1 diff --git a/components/freertos/queue.c b/components/freertos/queue.c index 92e182a620..248ae7c00a 100644 --- a/components/freertos/queue.c +++ b/components/freertos/queue.c @@ -483,14 +483,15 @@ int8_t *pcAllocatedBuffer; void* xQueueGetMutexHolder( QueueHandle_t xSemaphore ) { - void *pxReturn; + Queue_t * const pxQueue = ( Queue_t * ) xSemaphore; + void *pxReturn; /* This function is called by xSemaphoreGetMutexHolder(), and should not be called directly. Note: This is a good way of determining if the calling task is the mutex holder, but not a good way of determining the identity of the mutex holder, as the holder may change between the following critical section exiting and the function returning. */ - taskENTER_CRITICAL(); + taskENTER_CRITICAL(&pxQueue->mux); { if( ( ( Queue_t * ) xSemaphore )->uxQueueType == queueQUEUE_IS_MUTEX ) { @@ -501,7 +502,7 @@ int8_t *pcAllocatedBuffer; pxReturn = NULL; } } - taskEXIT_CRITICAL(); + taskEXIT_CRITICAL(&pxQueue->mux); return pxReturn; } /*lint !e818 xSemaphore cannot be a pointer to const because it is a typedef. */ From 700dbca4dbf42915702a8ce44272b635fb0e3b7e Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 24 Aug 2016 20:49:06 +0800 Subject: [PATCH 08/10] newlib locking: Remove lock table, much simpler implementation. --- components/esp32/syscalls.c | 122 +++++++++++++++--------------------- 1 file changed, 50 insertions(+), 72 deletions(-) diff --git a/components/esp32/syscalls.c b/components/esp32/syscalls.c index 92bc33374a..60af7f97c1 100644 --- a/components/esp32/syscalls.c +++ b/components/esp32/syscalls.c @@ -178,57 +178,42 @@ ssize_t _read_r(struct _reent *r, int fd, void * dst, size_t size) { /* Notes on our newlib lock implementation: * - * - lock_t is int. This value which is an index into a lock table entry, - * with a high bit flag for "lock initialised". - * - Lock table is a table of FreeRTOS mutexes. + * - Use FreeRTOS mutex semaphores as locks. + * - lock_t is int, but we store an xSemaphoreHandle there. * - Locks are no-ops until the FreeRTOS scheduler is running. - * - Writing to the lock table is protected by a spinlock. - * + * - Due to this, locks need to be lazily initialised the first time + * they are acquired. Initialisation/deinitialisation of locks is + * protected by lock_init_spinlock. + * - Race conditions around lazy initialisation (via lock_acquire) are + * protected against. + * - Anyone calling lock_close is reponsible for ensuring noone else + * is holding the lock at this time. + * - Race conditions between lock_close & lock_init (for the same lock) + * are the responsibility of the caller. */ -/* Maybe make this configurable? - It's MAXFDs plus a few static locks. */ -#define LOCK_TABLE_SIZE 32 +static portMUX_TYPE lock_init_spinlock = portMUX_INITIALIZER_UNLOCKED; -static xSemaphoreHandle lock_table[LOCK_TABLE_SIZE]; -static portMUX_TYPE lock_table_spinlock = portMUX_INITIALIZER_UNLOCKED; - -#define LOCK_INDEX_INITIALISED_FLAG (1<<31) - -/* Utility function to look up a particular lock in the lock table and - * return a pointer to its xSemaphoreHandle entry. */ -static inline IRAM_ATTR xSemaphoreHandle *get_lock_table_entry(_lock_t *lock) -{ - if (*lock & LOCK_INDEX_INITIALISED_FLAG) { - return &lock_table[*lock & ~LOCK_INDEX_INITIALISED_FLAG]; - } - return NULL; -} - -static inline IRAM_ATTR bool get_scheduler_started(void) -{ - int s = xTaskGetSchedulerState(); - return s != taskSCHEDULER_NOT_STARTED; -} - -/* Initialise the given lock by inserting a new mutex semaphore of - type mutex_type into the lock table. +/* Initialise the given lock by allocating a new mutex semaphore + as the _lock_t value. */ static void IRAM_ATTR lock_init_generic(_lock_t *lock, uint8_t mutex_type) { - if (!get_scheduler_started()) { - return; /* nothing to do until the scheduler is running */ + portENTER_CRITICAL(&lock_init_spinlock); + if (xTaskGetSchedulerState() == taskSCHEDULER_NOT_STARTED) { + /* nothing to do until the scheduler is running */ + *lock = 0; /* ensure lock is zeroed out, in case it's an automatic variable */ + portEXIT_CRITICAL(&lock_init_spinlock); + return; } - portENTER_CRITICAL(&lock_table_spinlock); - if (*lock & LOCK_INDEX_INITIALISED_FLAG) { + if (*lock) { /* Lock already initialised (either we didn't check earlier, or it got initialised while we were waiting for the spinlock.) */ - configASSERT(*get_lock_table_entry(lock) != NULL); } else { - /* Create a new semaphore and save it in the lock table. + /* Create a new semaphore this is a bit of an API violation, as we're calling the private function xQueueCreateMutex(x) directly instead of @@ -246,19 +231,9 @@ static void IRAM_ATTR lock_init_generic(_lock_t *lock, uint8_t mutex_type) { if (!new_sem) { abort(); /* No more semaphores available or OOM */ } - bool success = false; - for (int i = 0; i < LOCK_TABLE_SIZE && !success; i++) { - if (lock_table[i] == 0) { - lock_table[i] = new_sem; - *lock = i | LOCK_INDEX_INITIALISED_FLAG; - success = true; - } - } - if (!success) { - abort(); /* we have more locks than lock table entries */ - } + *lock = (_lock_t)new_sem; } - portEXIT_CRITICAL(&lock_table_spinlock); + portEXIT_CRITICAL(&lock_init_spinlock); } void IRAM_ATTR _lock_init(_lock_t *lock) { @@ -269,41 +244,39 @@ void IRAM_ATTR _lock_init_recursive(_lock_t *lock) { lock_init_generic(lock, queueQUEUE_TYPE_RECURSIVE_MUTEX); } -/* Free the mutex semaphore pointed to by *lock, and zero out - the entry in the lock table. +/* Free the mutex semaphore pointed to by *lock, and zero it out. Note that FreeRTOS doesn't account for deleting mutexes while they are held, and neither do we... so take care not to delete newlib locks while they may be held by other tasks! */ void IRAM_ATTR _lock_close(_lock_t *lock) { - if (*lock & LOCK_INDEX_INITIALISED_FLAG) { - portENTER_CRITICAL(&lock_table_spinlock); - xSemaphoreHandle *h = get_lock_table_entry(lock); + portENTER_CRITICAL(&lock_init_spinlock); + if (*lock) { + xSemaphoreHandle h = (xSemaphoreHandle)(*lock); #if (INCLUDE_xSemaphoreGetMutexHolder == 1) - configASSERT(xSemaphoreGetMutexHolder(*h) != NULL); /* mutex should not be held */ + configASSERT(xSemaphoreGetMutexHolder(h) == NULL); /* mutex should not be held */ #endif - vSemaphoreDelete(*h); - *h = NULL; - *lock = 0; - portEXIT_CRITICAL(&lock_table_spinlock); + vSemaphoreDelete(h); + h = NULL; } + portEXIT_CRITICAL(&lock_init_spinlock); } -/* Acquire the mutex semaphore indexed by lock, wait up to delay ticks. +/* Acquire the mutex semaphore for lock. wait up to delay ticks. mutex_type is queueQUEUE_TYPE_RECURSIVE_MUTEX or queueQUEUE_TYPE_MUTEX */ static int IRAM_ATTR lock_acquire_generic(_lock_t *lock, uint32_t delay, uint8_t mutex_type) { - xSemaphoreHandle *h = get_lock_table_entry(lock); + xSemaphoreHandle h = (xSemaphoreHandle)(*lock); if (!h) { - if (!get_scheduler_started()) { + if (xTaskGetSchedulerState() == taskSCHEDULER_NOT_STARTED) { return 0; /* locking is a no-op before scheduler is up, so this "succeeds" */ } /* lazy initialise lock - might have had a static initializer in newlib (that we don't use), or _lock_init might have been called before the scheduler was running... */ lock_init_generic(lock, mutex_type); } - h = get_lock_table_entry(lock); + h = (xSemaphoreHandle)(*lock); /* re-check after lock_init_generic */ configASSERT(h != NULL); BaseType_t success; @@ -313,18 +286,20 @@ static int IRAM_ATTR lock_acquire_generic(_lock_t *lock, uint32_t delay, uint8_t abort(); /* recursive mutexes make no sense in ISR context */ } BaseType_t higher_task_woken = false; - success = xSemaphoreTakeFromISR(*h, &higher_task_woken); + success = xSemaphoreTakeFromISR(h, &higher_task_woken); if (!success && delay > 0) { abort(); /* Tried to block on mutex from ISR, couldn't... rewrite your program to avoid libc interactions in ISRs! */ } - /* TODO: deal with higher_task_woken */ + if (higher_task_woken) { + portYIELD_FROM_ISR(); + } } else { /* In task context */ if (mutex_type == queueQUEUE_TYPE_RECURSIVE_MUTEX) { - success = xSemaphoreTakeRecursive(*h, delay); + success = xSemaphoreTakeRecursive(h, delay); } else { - success = xSemaphoreTake(*h, delay); + success = xSemaphoreTake(h, delay); } } @@ -347,11 +322,11 @@ int IRAM_ATTR _lock_try_acquire_recursive(_lock_t *lock) { return lock_acquire_generic(lock, 0, queueQUEUE_TYPE_RECURSIVE_MUTEX); } -/* Release the mutex semaphore indexed by lock. +/* Release the mutex semaphore for lock. mutex_type is queueQUEUE_TYPE_RECURSIVE_MUTEX or queueQUEUE_TYPE_MUTEX */ static void IRAM_ATTR lock_release_generic(_lock_t *lock, uint8_t mutex_type) { - xSemaphoreHandle *h = get_lock_table_entry(lock); + xSemaphoreHandle h = (xSemaphoreHandle)(*lock); if (h == NULL) { /* This is probably because the scheduler isn't running yet, or the scheduler just started running and some code was @@ -364,12 +339,15 @@ static void IRAM_ATTR lock_release_generic(_lock_t *lock, uint8_t mutex_type) { abort(); /* indicates logic bug, it shouldn't be possible to lock recursively in ISR */ } BaseType_t higher_task_woken = false; - xSemaphoreGiveFromISR(*h, &higher_task_woken); + xSemaphoreGiveFromISR(h, &higher_task_woken); + if (higher_task_woken) { + portYIELD_FROM_ISR(); + } } else { if (mutex_type == queueQUEUE_TYPE_RECURSIVE_MUTEX) { - xSemaphoreGiveRecursive(*h); + xSemaphoreGiveRecursive(h); } else { - xSemaphoreGive(*h); + xSemaphoreGive(h); } } } From 96b9649aa48877dade4de4ba255b9ebb661e95e0 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 25 Aug 2016 14:27:36 +0800 Subject: [PATCH 09/10] newlib locking: Fix bug w/ _lock_close not clearing semaphore handle --- components/esp32/syscalls.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/components/esp32/syscalls.c b/components/esp32/syscalls.c index 60af7f97c1..636e32670e 100644 --- a/components/esp32/syscalls.c +++ b/components/esp32/syscalls.c @@ -258,7 +258,7 @@ void IRAM_ATTR _lock_close(_lock_t *lock) { configASSERT(xSemaphoreGetMutexHolder(h) == NULL); /* mutex should not be held */ #endif vSemaphoreDelete(h); - h = NULL; + *lock = 0; } portEXIT_CRITICAL(&lock_init_spinlock); } @@ -275,9 +275,9 @@ static int IRAM_ATTR lock_acquire_generic(_lock_t *lock, uint32_t delay, uint8_t /* lazy initialise lock - might have had a static initializer in newlib (that we don't use), or _lock_init might have been called before the scheduler was running... */ lock_init_generic(lock, mutex_type); + h = (xSemaphoreHandle)(*lock); + configASSERT(h != NULL); } - h = (xSemaphoreHandle)(*lock); /* re-check after lock_init_generic */ - configASSERT(h != NULL); BaseType_t success; if (cpu_in_interrupt_context()) { From 00ea21f736fbe03345b1701a48739314bae46a89 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Thu, 25 Aug 2016 16:30:47 +0800 Subject: [PATCH 10/10] FreeRTOS: Convert portMUX_DEBUG to a configuration item --- components/freertos/Kconfig | 27 +++++++++++++++++++ .../freertos/include/freertos/portmacro.h | 14 +++------- components/freertos/port.c | 24 ++++++++--------- components/freertos/tasks.c | 8 +++--- 4 files changed, 46 insertions(+), 27 deletions(-) diff --git a/components/freertos/Kconfig b/components/freertos/Kconfig index 4c96e2a9cb..a9068d1174 100644 --- a/components/freertos/Kconfig +++ b/components/freertos/Kconfig @@ -155,4 +155,31 @@ config FREERTOS_BREAK_ON_SCHEDULER_START_JTAG If JTAG/OCD is connected, stop execution when the scheduler is started and the first task is executed. +menuconfig FREERTOS_DEBUG_INTERNALS + bool "Debug FreeRTOS internals" + default n + help + Enable this option to show the menu with internal FreeRTOS debugging features. + This option does not change any code by itself, it just shows/hides some options. + +if FREERTOS_DEBUG_INTERNALS + +config FREERTOS_PORTMUX_DEBUG + bool "Debug portMUX portENTER_CRITICAL/portEXIT_CRITICAL" + depends on FREERTOS_DEBUG_INTERNALS + default n + help + If enabled, debug information (including integrity checks) will be printed + to UART for the port-specific MUX implementation. + +config FREERTOS_PORTMUX_DEBUG_RECURSIVE + bool "Debug portMUX Recursion" + depends on FREERTOS_PORTMUX_DEBUG + default n + help + If enabled, additional debug information will be printed for recursive + portMUX usage. + +endif # FREERTOS_DEBUG_INTERNALS + endmenu diff --git a/components/freertos/include/freertos/portmacro.h b/components/freertos/include/freertos/portmacro.h index 807412fa88..908206eb37 100644 --- a/components/freertos/include/freertos/portmacro.h +++ b/components/freertos/include/freertos/portmacro.h @@ -120,13 +120,12 @@ typedef unsigned portBASE_TYPE UBaseType_t; #include "sdkconfig.h" -#define portMUX_DEBUG #define portFIRST_TASK_HOOK CONFIG_FREERTOS_BREAK_ON_SCHEDULER_START_JTAG typedef struct { volatile uint32_t mux; -#ifdef portMUX_DEBUG +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG const char *lastLockedFn; int lastLockedLine; #endif @@ -151,7 +150,7 @@ typedef struct { #define portMUX_VAL_SHIFT 0 //Keep this in sync with the portMUX_TYPE struct definition please. -#ifdef portMUX_DEBUG +#ifndef CONFIG_FREERTOS_PORTMUX_DEBUG #define portMUX_INITIALIZER_UNLOCKED { \ .mux = portMUX_MAGIC_VAL|portMUX_FREE_VAL \ } @@ -170,13 +169,6 @@ typedef struct { #define portCRITICAL_NESTING_IN_TCB 1 - -/* -Enable this to enable mux debugging. With this on, the mux code will warn you for deadlocks -and double releases etc. -*/ -#define portMUX_DEBUG - /* Modifications to portENTER_CRITICAL: @@ -198,7 +190,7 @@ This all assumes that interrupts are either entirely disabled or enabled. Interr will break this scheme. */ void vPortCPUInitializeMutex(portMUX_TYPE *mux); -#ifdef portMUX_DEBUG +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG void vPortCPUAcquireMutex(portMUX_TYPE *mux, const char *function, int line); portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux, const char *function, int line); void vTaskEnterCritical( portMUX_TYPE *mux, const char *function, int line ); diff --git a/components/freertos/port.c b/components/freertos/port.c index bbe0837d5f..afa4978639 100644 --- a/components/freertos/port.c +++ b/components/freertos/port.c @@ -276,7 +276,7 @@ uint32_t uxPortCompareSet(volatile uint32_t *mux, uint32_t compare, uint32_t set * For kernel use: Initialize a per-CPU mux. Mux will be initialized unlocked. */ void vPortCPUInitializeMutex(portMUX_TYPE *mux) { -#ifdef portMUX_DEBUG +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG ets_printf("Initializing mux %p\n", mux); mux->lastLockedFn="(never locked)"; mux->lastLockedLine=-1; @@ -288,7 +288,7 @@ void vPortCPUInitializeMutex(portMUX_TYPE *mux) { /* * For kernel use: Acquire a per-CPU mux. Spinlocks, so don't hold on to these muxes for too long. */ -#ifdef portMUX_DEBUG +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG void vPortCPUAcquireMutex(portMUX_TYPE *mux, const char *fnName, int line) { #else void vPortCPUAcquireMutex(portMUX_TYPE *mux) { @@ -296,7 +296,7 @@ void vPortCPUAcquireMutex(portMUX_TYPE *mux) { uint32_t res; uint32_t recCnt; unsigned int irqStatus; -#ifdef portMUX_DEBUG +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG uint32_t cnt=(1<<16); if ( (mux->mux & portMUX_MAGIC_MASK) != portMUX_MAGIC_VAL ) { ets_printf("ERROR: vPortCPUAcquireMutex: mux %p is uninitialized (0x%X)! Called from %s line %d.\n", mux, mux->mux, fnName, line); @@ -313,21 +313,21 @@ void vPortCPUAcquireMutex(portMUX_TYPE *mux) { //Mux was already locked by us. Just bump the recurse count by one. recCnt=(res&portMUX_CNT_MASK)>>portMUX_CNT_SHIFT; recCnt++; -#ifdef portMUX_DEBUG_RECURSIVE +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG_RECURSIVE ets_printf("Recursive lock: recCnt=%d last non-recursive lock %s line %d, curr %s line %d\n", recCnt, mux->lastLockedFn, mux->lastLockedLine, fnName, line); #endif mux->mux=portMUX_MAGIC_VAL|(recCnt<lastLockedFn, mux->lastLockedLine, fnName, line); ets_printf("Mux value %X\n", mux->mux); } #endif } while (res!=portMUX_FREE_VAL); -#ifdef portMUX_DEBUG +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG if (res==portMUX_FREE_VAL) { //initial lock mux->lastLockedFn=fnName; mux->lastLockedLine=line; @@ -340,7 +340,7 @@ void vPortCPUAcquireMutex(portMUX_TYPE *mux) { * For kernel use: Release a per-CPU mux. Returns true if everything is OK, false if mux * was already unlocked or is locked by a different core. */ -#ifdef portMUX_DEBUG +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux, const char *fnName, int line) { #else portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux) { @@ -351,7 +351,7 @@ portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux) { portBASE_TYPE ret=pdTRUE; // ets_printf("Unlock %p\n", mux); irqStatus=portENTER_CRITICAL_NESTED(); -#ifdef portMUX_DEBUG +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG const char *lastLockedFn=mux->lastLockedFn; int lastLockedLine=mux->lastLockedLine; mux->lastLockedFn=fnName; @@ -362,13 +362,13 @@ portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux) { res=uxPortCompareSet(&mux->mux, (xPortGetCoreID()<>portMUX_VAL_SHIFT) != xPortGetCoreID() ) { -#ifdef portMUX_DEBUG +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG ets_printf("ERROR: vPortCPUReleaseMutex: mux %p wasn't locked by this core (%d) but by core %d (ret=%x, mux=%x).\n", mux, xPortGetCoreID(), ((res&portMUX_VAL_MASK)>>portMUX_VAL_SHIFT), res, mux->mux); ets_printf("Last non-recursive lock %s line %d\n", lastLockedFn, lastLockedLine); ets_printf("Called by %s line %d\n", fnName, line); @@ -378,7 +378,7 @@ portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux) { //We locked this, but the reccount isn't zero. Decrease refcount and continue. recCnt=(res&portMUX_CNT_MASK)>>portMUX_CNT_SHIFT; recCnt--; -#ifdef portMUX_DEBUG_RECURSIVE +#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG_RECURSIVE ets_printf("Recursive unlock: recCnt=%d last locked %s line %d, curr %s line %d\n", recCnt, lastLockedFn, lastLockedLine, fnName, line); #endif mux->mux=portMUX_MAGIC_VAL|(recCnt<