From 5ab3de0bfa26dab4a5693b5758f9c8f8c6973df1 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Wed, 24 May 2017 16:29:11 -0500 Subject: [PATCH 1/9] Add function to check for ISR context Add the function core_util_in_isr() so code can determine if it is running in the context of an ISR. --- platform/mbed_critical.c | 16 ++++++++++++++++ platform/mbed_critical.h | 11 +++++++++++ 2 files changed, 27 insertions(+) diff --git a/platform/mbed_critical.c b/platform/mbed_critical.c index 75d97c14fc8..5068ab23d3d 100644 --- a/platform/mbed_critical.c +++ b/platform/mbed_critical.c @@ -37,6 +37,22 @@ bool core_util_are_interrupts_enabled(void) #endif } +bool core_util_is_isr_active(void) +{ +#if defined(__CORTEX_A9) + switch(__get_CPSR() & 0x1FU) { + case MODE_USR: + case MODE_SYS: + return false; + case MODE_SVC: + default: + return true; + } +#else + return (__get_IPSR() != 0U); +#endif +} + MBED_WEAK void core_util_critical_section_enter(void) { bool interrupts_disabled = !core_util_are_interrupts_enabled(); diff --git a/platform/mbed_critical.h b/platform/mbed_critical.h index dd4cbe6fb33..8aa314a8e99 100644 --- a/platform/mbed_critical.h +++ b/platform/mbed_critical.h @@ -41,6 +41,17 @@ extern "C" { */ bool core_util_are_interrupts_enabled(void); +/** Determine if this code is executing from an interrupt + * + * This function can be called to determine if the code is running on interrupt context. + * @note + * NOTE: + * This function works for both cortex-A and cortex-M, although the underlyng implementation + * differs. + * @return true if in an isr, false otherwise + */ +bool core_util_is_isr_active(void); + /** Mark the start of a critical section * * This function should be called to mark the start of a critical section of code. From b44e6f8a16ccee4bd81deb0922eabd4075a226b1 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Wed, 24 May 2017 15:12:52 -0500 Subject: [PATCH 2/9] Assert that file IO is not used in ISRs Trigger an assert if a file is read from or written to from an interrupt handler or critical section. This includes using printf from an interrupt handler or critical section. This makes failures due to use in incorrect context deterministic and easier to locate. This feature is enabled by defining MBED_TRAP_ERRORS_ENABLED to 1 or by using the debug profile. --- platform/mbed_retarget.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/platform/mbed_retarget.cpp b/platform/mbed_retarget.cpp index 739d6aa7173..b4328b14c5f 100644 --- a/platform/mbed_retarget.cpp +++ b/platform/mbed_retarget.cpp @@ -23,6 +23,7 @@ #include "platform/PlatformMutex.h" #include "platform/mbed_error.h" #include "platform/mbed_stats.h" +#include "platform/mbed_critical.h" #if MBED_CONF_FILESYSTEM_PRESENT #include "filesystem/FileSystem.h" #include "filesystem/File.h" @@ -309,6 +310,12 @@ extern "C" int PREFIX(_write)(FILEHANDLE fh, const unsigned char *buffer, unsign #endif int n; // n is the number of bytes written +#if defined(MBED_TRAP_ERRORS_ENABLED) && MBED_TRAP_ERRORS_ENABLED + if (core_util_is_isr_active() || !core_util_are_interrupts_enabled()) { + error("Error - writing to a file in an ISR or critical section\r\n"); + } +#endif + if (fh < 3) { #if DEVICE_SERIAL if (!stdio_uart_inited) init_serial(); @@ -353,6 +360,12 @@ extern "C" int PREFIX(_read)(FILEHANDLE fh, unsigned char *buffer, unsigned int #endif int n; // n is the number of bytes read +#if defined(MBED_TRAP_ERRORS_ENABLED) && MBED_TRAP_ERRORS_ENABLED + if (core_util_is_isr_active() || !core_util_are_interrupts_enabled()) { + error("Error - reading from a file in an ISR or critical section\r\n"); + } +#endif + if (fh < 3) { // only read a character at a time from stdin #if DEVICE_SERIAL From e48e599a9dade33b5caadf749ef7b270cfda550e Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Thu, 1 Jun 2017 15:18:52 -0500 Subject: [PATCH 3/9] Fix RTOS function prototypes by adding extern "C" Wrap the file mbed_rtos_storage.h in extern "C". This allows the functions inside rtx_lib.h to have correct definitions when included in a C++ file. This is required for the RTX5 error trapping. --- rtos/mbed_rtos_storage.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/rtos/mbed_rtos_storage.h b/rtos/mbed_rtos_storage.h index 270e57d4de9..50e92273701 100644 --- a/rtos/mbed_rtos_storage.h +++ b/rtos/mbed_rtos_storage.h @@ -22,6 +22,10 @@ #ifndef MBED_RTOS_STORAGE_H #define MBED_RTOS_STORAGE_H +#ifdef __cplusplus +extern "C" { +#endif + /** \addtogroup rtos */ /** @{*/ @@ -47,6 +51,10 @@ typedef os_event_flags_t mbed_rtos_storage_event_flags_t; typedef os_message_t mbed_rtos_storage_message_t; typedef os_timer_t mbed_rtos_storage_timer_t; +#ifdef __cplusplus +} +#endif + #endif /** @}*/ From 770ad616dd8a74052d59456f3faaeb4d5c8e7ccd Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Thu, 1 Jun 2017 15:19:13 -0500 Subject: [PATCH 4/9] Trap on RTX errors if enabled If MBED_TRAP_ERRORS_ENABLED is defined to 1 then trap on RTX errors. This includes using mutexes in ISR context. --- platform/mbed_retarget.cpp | 64 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/platform/mbed_retarget.cpp b/platform/mbed_retarget.cpp index b4328b14c5f..b49bba39f64 100644 --- a/platform/mbed_retarget.cpp +++ b/platform/mbed_retarget.cpp @@ -1046,3 +1046,67 @@ void operator delete[](void *ptr) free(ptr); } } + +#if defined(MBED_CONF_RTOS_PRESENT) && defined(MBED_TRAP_ERRORS_ENABLED) && MBED_TRAP_ERRORS_ENABLED + +static const char* error_msg(int32_t status) +{ + switch (status) { + case osError: + return "Unspecified RTOS error"; + case osErrorTimeout: + return "Operation not completed within the timeout period"; + case osErrorResource: + return "Resource not available"; + case osErrorParameter: + return "Parameter error"; + case osErrorNoMemory: + return "System is out of memory"; + case osErrorISR: + return "Not allowed in ISR context"; + default: + return "Unknown"; + } +} + +extern "C" void EvrRtxKernelError (int32_t status) +{ + error("Kernel error %i: %s\r\n", status, error_msg(status)); +} + +extern "C" void EvrRtxThreadError (osThreadId_t thread_id, int32_t status) +{ + error("Thread %p error %i: %s\r\n", thread_id, status, error_msg(status)); +} + +extern "C" void EvrRtxTimerError (osTimerId_t timer_id, int32_t status) +{ + error("Timer %p error %i: %s\r\n", timer_id, status, error_msg(status)); +} + +extern "C" void EvrRtxEventFlagsError (osEventFlagsId_t ef_id, int32_t status) +{ + error("Event %p error %i: %s\r\n", ef_id, status, error_msg(status)); +} + +extern "C" void EvrRtxMutexError (osMutexId_t mutex_id, int32_t status) +{ + error("Mutex %p error %i: %s\r\n", mutex_id, status, error_msg(status)); +} + +extern "C" void EvrRtxSemaphoreError (osSemaphoreId_t semaphore_id, int32_t status) +{ + error("Semaphore %p error %i\r\n", semaphore_id, status); +} + +extern "C" void EvrRtxMemoryPoolError (osMemoryPoolId_t mp_id, int32_t status) +{ + error("Memory Pool %p error %i\r\n", mp_id, status); +} + +extern "C" void EvrRtxMessageQueueError (osMessageQueueId_t mq_id, int32_t status) +{ + error("Message Queue %p error %i\r\n", mq_id, status); +} + +#endif From a84142fc4e66deef605a15da72b417565f87a39a Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Thu, 1 Jun 2017 15:53:06 -0500 Subject: [PATCH 5/9] Prevent recursive call to error() Only allow error to be called once. This prevents a loop where error() calls exit() which in turn triggers another call to exit(). --- platform/mbed_error.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/platform/mbed_error.c b/platform/mbed_error.c index 686355f933c..37422033c00 100644 --- a/platform/mbed_error.c +++ b/platform/mbed_error.c @@ -23,7 +23,16 @@ #include #endif +static uint8_t error_in_progress = 0; + WEAK void error(const char* format, ...) { + + // Prevent recursion if error is called again + if (error_in_progress) { + return; + } + error_in_progress = 1; + #ifndef NDEBUG va_list arg; va_start(arg, format); From 96bd943ea210569adb002a8676d62cd2dd021496 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Thu, 1 Jun 2017 17:58:40 -0500 Subject: [PATCH 6/9] RTX5: Enable priority inheritance and robust mode Add the attribute flash to enable priority inheritance and robust mode. The robust flag allows mutexes held by terminated threads to be properly released. --- .../arm_hal_interrupt.c | 2 +- .../ns_event_loop.c | 2 +- rtos/Mutex.cpp | 2 +- rtos/mbed_boot.c | 16 ++++++++-------- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/arm_hal_interrupt.c b/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/arm_hal_interrupt.c index 2110f8efa66..91485d1a14b 100644 --- a/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/arm_hal_interrupt.c +++ b/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/arm_hal_interrupt.c @@ -13,7 +13,7 @@ static uint8_t sys_irq_disable_counter; static mbed_rtos_storage_mutex_t critical_mutex; static const osMutexAttr_t critical_mutex_attr = { .name = "nanostack_critical_mutex", - .attr_bits = osMutexRecursive, + .attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust, .cb_mem = &critical_mutex, .cb_size = sizeof critical_mutex, }; diff --git a/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/ns_event_loop.c b/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/ns_event_loop.c index 27c02b5a815..f9d136d9096 100644 --- a/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/ns_event_loop.c +++ b/features/FEATURE_COMMON_PAL/nanostack-hal-mbed-cmsis-rtos/ns_event_loop.c @@ -30,7 +30,7 @@ static osThreadId_t event_thread_id; static mbed_rtos_storage_mutex_t event_mutex; static const osMutexAttr_t event_mutex_attr = { .name = "nanostack_event_mutex", - .attr_bits = osMutexRecursive, + .attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust, .cb_mem = &event_mutex, .cb_size = sizeof event_mutex, }; diff --git a/rtos/Mutex.cpp b/rtos/Mutex.cpp index ade6ebb4a87..359af105368 100644 --- a/rtos/Mutex.cpp +++ b/rtos/Mutex.cpp @@ -44,7 +44,7 @@ void Mutex::constructor(const char *name) _attr.name = name ? name : "aplication_unnamed_mutex"; _attr.cb_mem = &_obj_mem; _attr.cb_size = sizeof(_obj_mem); - _attr.attr_bits = osMutexRecursive; + _attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust; _id = osMutexNew(&_attr); MBED_ASSERT(_id); } diff --git a/rtos/mbed_boot.c b/rtos/mbed_boot.c index f2620497d8d..93029898fb5 100644 --- a/rtos/mbed_boot.c +++ b/rtos/mbed_boot.c @@ -363,7 +363,7 @@ void $Sub$$__cpp_initialize__aeabi_(void) void pre_main() { singleton_mutex_attr.name = "singleton_mutex"; - singleton_mutex_attr.attr_bits = osMutexRecursive; + singleton_mutex_attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust; singleton_mutex_attr.cb_size = sizeof(singleton_mutex_obj); singleton_mutex_attr.cb_mem = &singleton_mutex_obj; singleton_mutex_id = osMutexNew(&singleton_mutex_attr); @@ -383,7 +383,7 @@ extern int main(int argc, char* argv[]); void pre_main (void) { singleton_mutex_attr.name = "singleton_mutex"; - singleton_mutex_attr.attr_bits = osMutexRecursive; + singleton_mutex_attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust; singleton_mutex_attr.cb_size = sizeof(singleton_mutex_obj); singleton_mutex_attr.cb_mem = &singleton_mutex_obj; singleton_mutex_id = osMutexNew(&singleton_mutex_attr); @@ -440,19 +440,19 @@ int __wrap_main(void) { void pre_main(void) { singleton_mutex_attr.name = "singleton_mutex"; - singleton_mutex_attr.attr_bits = osMutexRecursive; + singleton_mutex_attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust; singleton_mutex_attr.cb_size = sizeof(singleton_mutex_obj); singleton_mutex_attr.cb_mem = &singleton_mutex_obj; singleton_mutex_id = osMutexNew(&singleton_mutex_attr); malloc_mutex_attr.name = "malloc_mutex"; - malloc_mutex_attr.attr_bits = osMutexRecursive; + malloc_mutex_attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust; malloc_mutex_attr.cb_size = sizeof(malloc_mutex_obj); malloc_mutex_attr.cb_mem = &malloc_mutex_obj; malloc_mutex_id = osMutexNew(&malloc_mutex_attr); env_mutex_attr.name = "env_mutex"; - env_mutex_attr.attr_bits = osMutexRecursive; + env_mutex_attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust; env_mutex_attr.cb_size = sizeof(env_mutex_obj); env_mutex_attr.cb_mem = &env_mutex_obj; env_mutex_id = osMutexNew(&env_mutex_attr); @@ -526,7 +526,7 @@ static uint8_t low_level_init_needed; void pre_main(void) { singleton_mutex_attr.name = "singleton_mutex"; - singleton_mutex_attr.attr_bits = osMutexRecursive; + singleton_mutex_attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust; singleton_mutex_attr.cb_size = sizeof(singleton_mutex_obj); singleton_mutex_attr.cb_mem = &singleton_mutex_obj; singleton_mutex_id = osMutexNew(&singleton_mutex_attr); @@ -583,7 +583,7 @@ void __iar_system_Mtxinit(__iar_Rmtx *mutex) /* Initialize a system lock */ attr.name = "system_mutex"; attr.cb_mem = &std_mutex_sys[index]; attr.cb_size = sizeof(std_mutex_sys[index]); - attr.attr_bits = osMutexRecursive; + attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust; std_mutex_id_sys[index] = osMutexNew(&attr); *mutex = (__iar_Rmtx*)&std_mutex_id_sys[index]; return; @@ -619,7 +619,7 @@ void __iar_file_Mtxinit(__iar_Rmtx *mutex) /* Initialize a file lock */ attr.name = "file_mutex"; attr.cb_mem = &std_mutex_file[index]; attr.cb_size = sizeof(std_mutex_file[index]); - attr.attr_bits = osMutexRecursive; + attr.attr_bits = osMutexRecursive | osMutexPrioInherit | osMutexRobust; std_mutex_id_file[index] = osMutexNew(&attr); *mutex = (__iar_Rmtx*)&std_mutex_id_file[index]; return; From cabc1e09112c09747b3e44df5bcb0a511c9c8311 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Thu, 1 Jun 2017 17:59:44 -0500 Subject: [PATCH 7/9] Fix Thread class synchronization Prevent osTheadTerminate from being called on an already terminated thread. Also make sure the thread termination process is properly synchronized. --- rtos/Thread.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/rtos/Thread.cpp b/rtos/Thread.cpp index 990a1260628..cd3364861c8 100644 --- a/rtos/Thread.cpp +++ b/rtos/Thread.cpp @@ -103,7 +103,7 @@ osStatus Thread::start(Callback task) { } osStatus Thread::terminate() { - osStatus_t ret; + osStatus_t ret = osOK; _mutex.lock(); // Set the Thread's tid to NULL and @@ -112,11 +112,11 @@ osStatus Thread::terminate() { osThreadId_t local_id = _tid; _join_sem.release(); _tid = (osThreadId_t)NULL; - _finished = true; + if (!_finished) { + _finished = true; + ret = osThreadTerminate(local_id); + } _mutex.unlock(); - - ret = osThreadTerminate(local_id); - return ret; } @@ -352,8 +352,8 @@ void Thread::_thunk(void * thread_ptr) t->_mutex.lock(); t->_tid = (osThreadId)NULL; t->_finished = true; - t->_mutex.unlock(); t->_join_sem.release(); + // rtos will release the mutex automatically } } From ee2be3f90c721a9927b0fc43c65860b8a52310e2 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Thu, 1 Jun 2017 18:35:29 -0500 Subject: [PATCH 8/9] Ignore deprecated storage test Add a .mbedignore to the storage_abstraction test since this is deprecated. --- TESTS/storage_abstraction/.mbedignore | 1 + 1 file changed, 1 insertion(+) create mode 100644 TESTS/storage_abstraction/.mbedignore diff --git a/TESTS/storage_abstraction/.mbedignore b/TESTS/storage_abstraction/.mbedignore new file mode 100644 index 00000000000..72e8ffc0db8 --- /dev/null +++ b/TESTS/storage_abstraction/.mbedignore @@ -0,0 +1 @@ +* From 737c5a9ceadf33cd46ad90e9f1e8c0b5aa550314 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Thu, 1 Jun 2017 19:10:34 -0500 Subject: [PATCH 9/9] Trap on errors when building with debug profile Define MBED_TRAP_ERRORS_ENABLED to 1 for the debug profile so errors are obvious when building as debug. --- tools/profiles/debug.json | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tools/profiles/debug.json b/tools/profiles/debug.json index 95ab1a10862..1f8c5bce451 100644 --- a/tools/profiles/debug.json +++ b/tools/profiles/debug.json @@ -5,7 +5,8 @@ "-fmessage-length=0", "-fno-exceptions", "-fno-builtin", "-ffunction-sections", "-fdata-sections", "-funsigned-char", "-MMD", "-fno-delete-null-pointer-checks", - "-fomit-frame-pointer", "-O0", "-g3", "-DMBED_DEBUG"], + "-fomit-frame-pointer", "-O0", "-g3", "-DMBED_DEBUG", + "-DMBED_TRAP_ERRORS_ENABLED=1"], "asm": ["-x", "assembler-with-cpp"], "c": ["-std=gnu99"], "cxx": ["-std=gnu++98", "-fno-rtti", "-Wvla"], @@ -17,7 +18,8 @@ "ARM": { "common": ["-c", "--gnu", "-Otime", "--split_sections", "--apcs=interwork", "--brief_diagnostics", "--restrict", - "--multibyte_chars", "-O0", "-g", "-DMBED_DEBUG"], + "--multibyte_chars", "-O0", "-g", "-DMBED_DEBUG", + "-DMBED_TRAP_ERRORS_ENABLED=1"], "asm": [], "c": ["--md", "--no_depend_system_headers", "--c99", "-D__ASSERT_MSG"], "cxx": ["--cpp", "--no_rtti", "--no_vla"], @@ -27,7 +29,8 @@ "common": ["-c", "--gnu", "-Otime", "--split_sections", "--apcs=interwork", "--brief_diagnostics", "--restrict", "--multibyte_chars", "-O0", "-D__MICROLIB", "-g", - "--library_type=microlib", "-DMBED_RTOS_SINGLE_THREAD", "-DMBED_DEBUG"], + "--library_type=microlib", "-DMBED_RTOS_SINGLE_THREAD", "-DMBED_DEBUG", + "-DMBED_TRAP_ERRORS_ENABLED=1"], "asm": [], "c": ["--md", "--no_depend_system_headers", "--c99", "-D__ASSERT_MSG"], "cxx": ["--cpp", "--no_rtti", "--no_vla"], @@ -36,7 +39,8 @@ "IAR": { "common": [ "--no_wrap_diagnostics", "-e", - "--diag_suppress=Pa050,Pa084,Pa093,Pa082", "-On", "-r", "-DMBED_DEBUG"], + "--diag_suppress=Pa050,Pa084,Pa093,Pa082", "-On", "-r", "-DMBED_DEBUG", + "-DMBED_TRAP_ERRORS_ENABLED=1"], "asm": [], "c": ["--vla"], "cxx": ["--guard_calls", "--no_static_destruction"],