Furi: A Lot of Fixes (#3942)

- BT Service: cleanup code - Dialog: correct release order in file browser - Rpc: rollback to pre #3881 state - Kernel: fix inverted behavior in furi_kernel_is_running - Log: properly take mutex when kernel is not running - Thread: rework tread control block scrubbing procedure, ensure that we don't do stupid things in idle task, add new priority for init task - Timer: add control queue flush method, force flush on stop - Furi: system init task now performs thread scrubbing - BleGlue: add some extra checks - FreeRTOSConfig: fix bunch of issues that were preventing configuration from being properly applied and cleanup
flipperdevices · Oct 14, 2024 · 5190aac · 5190aac
1 parent 344118c
commit 5190aac
Show file tree

Hide file tree

Showing 19 changed files with 205 additions and 123 deletions.
diff --git a/applications/services/bt/bt_service/bt.c b/applications/services/bt/bt_service/bt.c
@@ -430,13 +430,11 @@ static void bt_change_profile(Bt* bt, BtMessage* message) {
             *message->profile_instance = NULL;
         }
     }
-    if(message->lock) api_lock_unlock(message->lock);
 }
 
-static void bt_close_connection(Bt* bt, BtMessage* message) {
+static void bt_close_connection(Bt* bt) {
     bt_close_rpc_connection(bt);
     furi_hal_bt_stop_advertising();
-    if(message->lock) api_lock_unlock(message->lock);
 }
 
 static void bt_apply_settings(Bt* bt) {
@@ -484,19 +482,13 @@ static void bt_load_settings(Bt* bt) {
 }
 
 static void bt_handle_get_settings(Bt* bt, BtMessage* message) {
-    furi_assert(message->lock);
     *message->data.settings = bt->bt_settings;
-    api_lock_unlock(message->lock);
 }
 
 static void bt_handle_set_settings(Bt* bt, BtMessage* message) {
-    furi_assert(message->lock);
     bt->bt_settings = *message->data.csettings;
-
     bt_apply_settings(bt);
     bt_settings_save(&bt->bt_settings);
-
-    api_lock_unlock(message->lock);
 }
 
 static void bt_handle_reload_keys_settings(Bt* bt) {
@@ -548,6 +540,12 @@ int32_t bt_srv(void* p) {
     while(1) {
         furi_check(
             furi_message_queue_get(bt->message_queue, &message, FuriWaitForever) == FuriStatusOk);
+        FURI_LOG_D(
+            TAG,
+            "call %d, lock 0x%p, result 0x%p",
+            message.type,
+            (void*)message.lock,
+            (void*)message.result);
         if(message.type == BtMessageTypeUpdateStatus) {
             // Update view ports
             bt_statusbar_update(bt);
@@ -571,7 +569,7 @@ int32_t bt_srv(void* p) {
         } else if(message.type == BtMessageTypeSetProfile) {
             bt_change_profile(bt, &message);
         } else if(message.type == BtMessageTypeDisconnect) {
-            bt_close_connection(bt, &message);
+            bt_close_connection(bt);
         } else if(message.type == BtMessageTypeForgetBondedDevices) {
             bt_keys_storage_delete(bt->keys_storage);
         } else if(message.type == BtMessageTypeGetSettings) {
@@ -581,6 +579,8 @@ int32_t bt_srv(void* p) {
         } else if(message.type == BtMessageTypeReloadKeysSettings) {
             bt_handle_reload_keys_settings(bt);
         }
+
+        if(message.lock) api_lock_unlock(message.lock);
     }
 
     return 0;

diff --git a/applications/services/dialogs/dialogs_module_file_browser.c b/applications/services/dialogs/dialogs_module_file_browser.c
@@ -54,11 +54,14 @@ bool dialogs_app_process_module_file_browser(const DialogsAppMessageDataFileBrow
     ret = file_browser_context->result;
 
     view_holder_set_view(view_holder, NULL);
-    view_holder_free(view_holder);
     file_browser_stop(file_browser);
+
     file_browser_free(file_browser);
+    view_holder_free(view_holder);
+
     api_lock_free(file_browser_context->lock);
     free(file_browser_context);
+
     furi_record_close(RECORD_GUI);
 
     return ret;

diff --git a/applications/services/rpc/rpc.c b/applications/services/rpc/rpc.c
@@ -67,7 +67,7 @@ static RpcSystemCallbacks rpc_systems[] = {
 struct RpcSession {
     Rpc* rpc;
 
-    FuriThreadId thread_id;
+    FuriThread* thread;
 
     RpcHandlerDict_t handlers;
     FuriStreamBuffer* stream;
@@ -172,7 +172,7 @@ size_t rpc_session_feed(
 
     size_t bytes_sent = furi_stream_buffer_send(session->stream, encoded_bytes, size, timeout);
 
-    furi_thread_flags_set(session->thread_id, RpcEvtNewData);
+    furi_thread_flags_set(furi_thread_get_id(session->thread), RpcEvtNewData);
 
     return bytes_sent;
 }
@@ -220,7 +220,7 @@ bool rpc_pb_stream_read(pb_istream_t* istream, pb_byte_t* buf, size_t count) {
                     break;
                 } else {
                     /* Save disconnect flag and continue reading buffer */
-                    furi_thread_flags_set(session->thread_id, RpcEvtDisconnect);
+                    furi_thread_flags_set(furi_thread_get_id(session->thread), RpcEvtDisconnect);
                 }
             } else if(flags & RpcEvtNewData) {
                 // Just wake thread up
@@ -347,32 +347,37 @@ static int32_t rpc_session_worker(void* context) {
     return 0;
 }
 
-static void rpc_session_thread_release_callback(
-    FuriThread* thread,
-    FuriThreadState thread_state,
-    void* context) {
-    if(thread_state == FuriThreadStateStopped) {
-        RpcSession* session = (RpcSession*)context;
+static void rpc_session_thread_pending_callback(void* context, uint32_t arg) {
+    UNUSED(arg);
+    RpcSession* session = (RpcSession*)context;
 
-        for(size_t i = 0; i < COUNT_OF(rpc_systems); ++i) {
-            if(rpc_systems[i].free) {
-                (rpc_systems[i].free)(session->system_contexts[i]);
-            }
-        }
-        free(session->system_contexts);
-        free(session->decoded_message);
-        RpcHandlerDict_clear(session->handlers);
-        furi_stream_buffer_free(session->stream);
-
-        furi_mutex_acquire(session->callbacks_mutex, FuriWaitForever);
-        if(session->terminated_callback) {
-            session->terminated_callback(session->context);
+    for(size_t i = 0; i < COUNT_OF(rpc_systems); ++i) {
+        if(rpc_systems[i].free) {
+            (rpc_systems[i].free)(session->system_contexts[i]);
         }
-        furi_mutex_release(session->callbacks_mutex);
+    }
+    free(session->system_contexts);
+    free(session->decoded_message);
+    RpcHandlerDict_clear(session->handlers);
+    furi_stream_buffer_free(session->stream);
+
+    furi_mutex_acquire(session->callbacks_mutex, FuriWaitForever);
+    if(session->terminated_callback) {
+        session->terminated_callback(session->context);
+    }
+    furi_mutex_release(session->callbacks_mutex);
+
+    furi_mutex_free(session->callbacks_mutex);
+    furi_thread_join(session->thread);
+    furi_thread_free(session->thread);
+    free(session);
+}
 
-        furi_mutex_free(session->callbacks_mutex);
-        furi_thread_free(thread);
-        free(session);
+static void
+    rpc_session_thread_state_callback(FuriThread* thread, FuriThreadState state, void* context) {
+    UNUSED(thread);
+    if(state == FuriThreadStateStopped) {
+        furi_timer_pending_callback(rpc_session_thread_pending_callback, context, 0);
     }
 }
 
@@ -404,14 +409,12 @@ RpcSession* rpc_session_open(Rpc* rpc, RpcOwner owner) {
     };
     rpc_add_handler(session, PB_Main_stop_session_tag, &rpc_handler);
 
-    FuriThread* thread =
-        furi_thread_alloc_ex("RpcSessionWorker", 3072, rpc_session_worker, session);
-    session->thread_id = furi_thread_get_id(thread);
+    session->thread = furi_thread_alloc_ex("RpcSessionWorker", 3072, rpc_session_worker, session);
 
-    furi_thread_set_state_context(thread, session);
-    furi_thread_set_state_callback(thread, rpc_session_thread_release_callback);
+    furi_thread_set_state_context(session->thread, session);
+    furi_thread_set_state_callback(session->thread, rpc_session_thread_state_callback);
 
-    furi_thread_start(thread);
+    furi_thread_start(session->thread);
 
     return session;
 }
@@ -423,7 +426,7 @@ void rpc_session_close(RpcSession* session) {
     rpc_session_set_send_bytes_callback(session, NULL);
     rpc_session_set_close_callback(session, NULL);
     rpc_session_set_buffer_is_empty_callback(session, NULL);
-    furi_thread_flags_set(session->thread_id, RpcEvtDisconnect);
+    furi_thread_flags_set(furi_thread_get_id(session->thread), RpcEvtDisconnect);
 }
 
 void rpc_on_system_start(void* p) {

diff --git a/furi/core/kernel.c b/furi/core/kernel.c
@@ -33,7 +33,7 @@ bool furi_kernel_is_irq_or_masked(void) {
 }
 
 bool furi_kernel_is_running(void) {
-    return xTaskGetSchedulerState() != taskSCHEDULER_RUNNING;
+    return xTaskGetSchedulerState() == taskSCHEDULER_RUNNING;
 }
 
 int32_t furi_kernel_lock(void) {
@@ -129,6 +129,8 @@ uint32_t furi_kernel_get_tick_frequency(void) {
 
 void furi_delay_tick(uint32_t ticks) {
     furi_check(!furi_kernel_is_irq_or_masked());
+    furi_check(furi_thread_get_current_id() != xTaskGetIdleTaskHandle());
+
     if(ticks == 0U) {
         taskYIELD();
     } else {
@@ -138,6 +140,7 @@ void furi_delay_tick(uint32_t ticks) {
 
 FuriStatus furi_delay_until_tick(uint32_t tick) {
     furi_check(!furi_kernel_is_irq_or_masked());
+    furi_check(furi_thread_get_current_id() != xTaskGetIdleTaskHandle());
 
     TickType_t tcnt, delay;
     FuriStatus stat;

diff --git a/furi/core/log.c b/furi/core/log.c
@@ -108,10 +108,17 @@ void furi_log_puts(const char* data) {
 }
 
 void furi_log_print_format(FuriLogLevel level, const char* tag, const char* format, ...) {
-    if(level <= furi_log.log_level &&
-       furi_mutex_acquire(furi_log.mutex, FuriWaitForever) == FuriStatusOk) {
-        FuriString* string;
-        string = furi_string_alloc();
+    do {
+        if(level > furi_log.log_level) {
+            break;
+        }
+
+        if(furi_mutex_acquire(furi_log.mutex, furi_kernel_is_running() ? FuriWaitForever : 0) !=
+           FuriStatusOk) {
+            break;
+        }
+
+        FuriString* string = furi_string_alloc();
 
         const char* color = _FURI_LOG_CLR_RESET;
         const char* log_letter = " ";
@@ -157,7 +164,7 @@ void furi_log_print_format(FuriLogLevel level, const char* tag, const char* form
         furi_log_puts("\r\n");
 
         furi_mutex_release(furi_log.mutex);
-    }
+    } while(0);
 }
 
 void furi_log_print_raw_format(FuriLogLevel level, const char* format, ...) {

diff --git a/furi/core/thread.c b/furi/core/thread.c
@@ -1,6 +1,7 @@
-#include "thread.h"
+#include "thread_i.h"
 #include "thread_list_i.h"
 #include "kernel.h"
+#include "message_queue.h"
 #include "memmgr.h"
 #include "memmgr_heap.h"
 #include "check.h"
@@ -67,6 +68,8 @@ static_assert(offsetof(FuriThread, container) == 0);
 // Our idle priority should be equal to the one from FreeRTOS
 static_assert(FuriThreadPriorityIdle == tskIDLE_PRIORITY);
 
+static FuriMessageQueue* furi_thread_scrub_message_queue = NULL;
+
 static size_t __furi_thread_stdout_write(FuriThread* thread, const char* data, size_t size);
 static int32_t __furi_thread_stdout_flush(FuriThread* thread);
 
@@ -125,7 +128,9 @@ static void furi_thread_body(void* context) {
 
     furi_thread_set_state(thread, FuriThreadStateStopping);
 
-    vTaskDelete(NULL);
+    furi_message_queue_put(furi_thread_scrub_message_queue, &thread, FuriWaitForever);
+
+    vTaskSuspend(NULL);
     furi_thread_catch();
 }
 
@@ -159,6 +164,31 @@ static void furi_thread_init_common(FuriThread* thread) {
     }
 }
 
+void furi_thread_init(void) {
+    furi_thread_scrub_message_queue = furi_message_queue_alloc(8, sizeof(FuriThread*));
+}
+
+void furi_thread_scrub(void) {
+    FuriThread* thread_to_scrub = NULL;
+    while(true) {
+        furi_check(
+            furi_message_queue_get(
+                furi_thread_scrub_message_queue, &thread_to_scrub, FuriWaitForever) ==
+            FuriStatusOk);
+
+        TaskHandle_t task = (TaskHandle_t)thread_to_scrub;
+
+        // Delete task: FreeRTOS will remove task from all lists where it may be
+        vTaskDelete(task);
+        // Sanity check: ensure that local storage is ours and clear it
+        furi_check(pvTaskGetThreadLocalStoragePointer(task, 0) == thread_to_scrub);
+        vTaskSetThreadLocalStoragePointer(task, 0, NULL);
+
+        // Deliver thread stopped callback
+        furi_thread_set_state(thread_to_scrub, FuriThreadStateStopped);
+    }
+}
+
 FuriThread* furi_thread_alloc(void) {
     FuriThread* thread = malloc(sizeof(FuriThread));
 
@@ -358,16 +388,6 @@ void furi_thread_start(FuriThread* thread) {
             &thread->container) == (TaskHandle_t)thread);
 }
 
-void furi_thread_cleanup_tcb_event(TaskHandle_t task) {
-    FuriThread* thread = pvTaskGetThreadLocalStoragePointer(task, 0);
-    if(thread) {
-        // clear thread local storage
-        vTaskSetThreadLocalStoragePointer(task, 0, NULL);
-        furi_check(thread == (FuriThread*)task);
-        furi_thread_set_state(thread, FuriThreadStateStopped);
-    }
-}
-
 bool furi_thread_join(FuriThread* thread) {
     furi_check(thread);
     // Cannot join a service thread

diff --git a/furi/core/thread.h b/furi/core/thread.h
@@ -21,17 +21,18 @@ extern "C" {
  * Many of the FuriThread functions MUST ONLY be called when the thread is STOPPED.
  */
 typedef enum {
-    FuriThreadStateStopped, /**< Thread is stopped and is safe to release */
-    FuriThreadStateStopping, /**< Thread is stopping */
-    FuriThreadStateStarting, /**< Thread is starting */
-    FuriThreadStateRunning, /**< Thread is running */
+    FuriThreadStateStopped, /**< Thread is stopped and is safe to release. Event delivered from system init thread(TCB cleanup routine). It is safe to release thread instance. */
+    FuriThreadStateStopping, /**< Thread is stopping. Event delivered from child thread. */
+    FuriThreadStateStarting, /**< Thread is starting. Event delivered from parent(self) thread. */
+    FuriThreadStateRunning, /**< Thread is running. Event delivered from child thread. */
 } FuriThreadState;
 
 /**
  * @brief Enumeration of possible FuriThread priorities.
  */
 typedef enum {
     FuriThreadPriorityIdle = 0, /**< Idle priority */
+    FuriThreadPriorityInit = 4, /**< Init System Thread Priority */
     FuriThreadPriorityLowest = 14, /**< Lowest */
     FuriThreadPriorityLow = 15, /**< Low */
     FuriThreadPriorityNormal = 16, /**< Normal, system default */
@@ -77,13 +78,15 @@ typedef int32_t (*FuriThreadCallback)(void* context);
 typedef void (*FuriThreadStdoutWriteCallback)(const char* data, size_t size);
 
 /**
- * @brief State change callback function pointer type.
+ * @brief         State change callback function pointer type.
  *
- * The function to be used as a state callback MUST follow this signature.
+ *                The function to be used as a state callback MUST follow this
+ *                signature.
  *
- * @param[in] pointer to the FuriThread instance that changed the state
- * @param[in] state identifier of the state the thread has transitioned to
- * @param[in,out] context pointer to a user-specified object
+ * @param[in]     thread   to the FuriThread instance that changed the state
+ * @param[in]     state    identifier of the state the thread has transitioned
+ *                         to
+ * @param[in,out] context  pointer to a user-specified object
  */
 typedef void (*FuriThreadStateCallback)(FuriThread* thread, FuriThreadState state, void* context);
 

diff --git a/furi/core/thread_i.h b/furi/core/thread_i.h
@@ -0,0 +1,7 @@
+#pragma once
+
+#include "thread.h"
+
+void furi_thread_init(void);
+
+void furi_thread_scrub(void);